HY-4737 Fix unloading, refactor and cleanup #15

robbecker-wf · 2017-04-10T16:58:59Z

Problem

Font unloading doesn't work well since it doesn't track uses for loaded fonts correctly across groups and doesn't handle the async possibility of loading/unloading when things are in progress.

Solution

Refactor to track uses per group, and wait for loading to finish before unloading.
I added dartdoc strings and turned doc generation and publishing back on.
I turned on the semver audit service.
I turned on nearly all of the analysis options and strong mode.

+10 instructions

Unit tests pass
run pub serve test and play with localhost:8080/unload.html

# Conflicts: # lib/font_face_observer.dart

aviary2-wf · 2017-04-10T16:59:03Z

Raven

Number of Findings: 0

rmconsole-wf · 2017-04-10T16:59:23Z

General Information

Ticket(s):

HY-4737 | remove

Code Review(s): #15

Additional Information

Reviewers: robbecker-wf, seanburke-wf, tomconnell-wf, stephenbush-wf
Watchlist Notifications: None

	When this pull is merged I will use the following information:
	Version: font_face_observer 2.0.0
	Release Ticket(s): HY-4693, HY-4694

Last updated on Friday, April 14 04:09 PM CST

robbecker-wf · 2017-04-14T00:14:36Z

smithy.yml

@@ -1,7 +1,7 @@
 project: static
 language: dart

-runner_image: drydock-prod.workiva.org/workiva/smithy-runner-dart:91483
+runner_image: drydock-prod.workiva.net/workiva/smithy-runner-dart:124921


update to Dart 1.22.1

tomconnell-wf · 2017-04-14T13:57:42Z

README.md

@@ -46,8 +46,14 @@ FontFaceObserver(
 Future<FontLoadResult> check() async

 // Load a font from the given URL by adding a font-face rule
-// returns the same Future<bool> from isLoaded()
+// returns the same Future<FontLoadResult> from check()


tomconnell-wf · 2017-04-14T14:00:03Z

README.md

+Future<int> FontFaceObserver.unloadGroup(String group);
+
+// Unload a specific font by key and group
+Future<bool> FontFaceObserver.unload(String key, String group);


Might want a unloadAll someday. Could be useful for mobile, to be called when receiving low memory warnings. 📦

I thought about unloadAll but consider it potentially unsafe to use since fonts may be loaded from different areas and shared. If one calls unload all, it may unload a font that some place else is actively using. Perhaps this could be carefully added in a followup.

tomconnell-wf · 2017-04-14T14:01:00Z

lib/data_uri.dart

+      {this.mimeType: 'application/octet-stream',
+      this.encoding,
+      this.data,
+      this.isDataBase64Encoded: true});


tomconnell-wf · 2017-04-14T14:02:38Z

lib/font_face_observer.dart

  final bool didTimeout;
+
+  /// Construct a new FontLoadResult


👎 says exactly what the code does.

truth. I'm looking at the dartdoc output and improving the readme and doc strings now.

tomconnell-wf · 2017-04-14T14:05:30Z

lib/font_face_observer.dart

-    return false;
-  }
+bool _isNullOrWhitespace(String s) {
+  return s == null || s.trim().isEmpty;


I know this is tiny, but you could use quiver.

I wanted to avoid that dependency here

tomconnell-wf · 2017-04-14T14:10:14Z

lib/font_face_observer.dart

+
+  /// Recalculcate the sum of total uses and total list of groups for this
+  /// font record and update the data-groups attribute on the style element.
+  void _recalc() {


Since this takes no parameters, the naming gives no clue to what is recalc'ed. _recalcGroupUses?

I'll dogpile on this while I am here. I almost feel like this should take a groupUses and return the groupData, then it could just be a function that doesn't have to mutate state. But, I'd better leave talk of functional programming to the pros.

I chose to rename to _updateGroupUseCounts. How's that feel?

tomconnell-wf · 2017-04-14T14:15:15Z

lib/font_face_observer.dart

+/// DOM or drawn into a canvas. If you need to load fonts with an authenticated
+/// request, you can handle the networking part yourself and then still use
+/// FontFaceObserver to detect when it is ready by passing in the font as a
+/// base64 data or Blob URI.


This is a nice doc string. It would be nice to have an example of how to get base64 data or the 'Blob URI'

Agree. I'm currently working on dartdoc. I'll add.

tomconnell-wf · 2017-04-14T14:19:17Z

lib/font_face_observer.dart

+    List<String> keysToRemove = new List<String>();
+    List<_FontRecord> records = new List<_FontRecord>();
+    for (String k in _loadedFonts.keys.toList()) {
+      // _loadedFonts.keys.forEach(await (k) async {


tomconnell-wf · 2017-04-14T14:20:33Z

lib/font_face_observer.dart

+      // _loadedFonts.keys.forEach(await (k) async {
+      _FontRecord record = _loadedFonts[k];
+      // wait for the load future to complete
+      await record.futureLoadResult;


Would it be better to have a callback when the future completes, instead of blocking on it?

tomconnell-wf · 2017-04-14T14:25:39Z

lib/font_face_observer.dart

+    // if we get here, the font load has timed out
+    // make sure the font is unloaded
+    _unloadFont(key, record);
+    // TODO return a failed future


Need to do this still?

tomconnell-wf · 2017-04-14T14:28:13Z

lib/font_face_observer.dart

+  /// for a given [family] at a certain [cssSize] (default 100px)
+  String _getStyle(String family, {String cssSize: '100px'}) {
+    String _stretch = supportsStretch ? stretch : '';
+    return '$style $weight $_stretch $cssSize $family';


Is the double space if stretch is '' going to be problem?

It should not be.

tomconnell-wf · 2017-04-14T14:30:01Z

lib/font_face_observer.dart

+  /// one of which will trigger a scroll event when the font loads and the
+  /// dimensions of the test string change. These rulers are checked periodically
+  /// waiting for the font to become available. The Timer is returned so it may
+  /// be cancelled and not check infinitely.


tomconnell-wf · 2017-04-14T14:31:18Z

lib/font_face_observer.dart

      widthSerif = width;
      _checkWidths();
    });

    _rulerSerif.setFont(_getStyle('"$family",AdobeBlank,serif'));

-    _rulerMonospace.onResize((width) {
+    _rulerMonospace.onResize((num width) {


Be nice to your code reviewers; let's turn on linting in a distinct pr, next time!

sorry, scope creeped this PR.

seanburke-wf · 2017-04-14T20:21:02Z

+1 SDK bump

semveraudit-wf · 2017-04-14T20:21:49Z

No audit for base 374f1c4

Powered by semver-audit-service. Please report any problems by filing an issue.
Browse public API.

seanburke-wf · 2017-04-14T20:25:35Z

@rmconsole-wf

semveraudit-wf · 2017-04-14T20:52:36Z

No audit for base 374f1c4

Powered by semver-audit-service. Please report any problems by filing an issue.
Browse public API.

seanburke-wf · 2017-04-14T20:53:41Z

+1 the changes

semveraudit-wf · 2017-04-14T21:00:57Z

No audit for base 374f1c4

Powered by semver-audit-service. Please report any problems by filing an issue.
Browse public API.

stephenbush-wf · 2017-04-14T21:03:19Z

+10 Seems legit!

run pub serve test and play with localhost:8080/unload.html

seanburke-wf · 2017-04-14T21:08:16Z

+1 on the comment change

seanburke-wf · 2017-04-14T21:08:52Z

QA +1

@Workiva/release-management-p for merge into master

rmconsole-wf · 2017-04-14T21:09:25Z

+1 from RM

robbecker-wf added 4 commits March 30, 2017 08:45

unloading is async now

56cdc49

not working very well

e8af816

fix demo

5b51283

Merge remote-tracking branch 'origin/master' into fix_unloading

9b1295f

# Conflicts: # lib/font_face_observer.dart

rmconsole-wf added the Merge Requirements Unmet label Apr 10, 2017

robbecker-wf added 5 commits April 11, 2017 00:42

unloading works much better

6323b4e

minor fixes

258ba6c

organized, delinted, cleaned up, so much

1f065cd

update readme

1b68364

update smithy dart image

27f9d96

robbecker-wf changed the title ~~Fix unloading~~ Fix unloading, refactor and cleanup Apr 13, 2017

rm-astro-wf changed the title ~~Fix unloading, refactor and cleanup~~ HY-4737 Fix unloading, refactor and cleanup Apr 13, 2017

cleaned up isLoaded, rolled into check()

dbbee29

robbecker-wf commented Apr 14, 2017

View reviewed changes

remove commented out code

e8d7beb

tomconnell-wf reviewed Apr 14, 2017

View reviewed changes

oh, set min dart SDK because semver audit needs 1.22.1 min

c9296ad

rmconsole-wf added Merge Requirements Unmet and removed Merge Requirements Met labels Apr 14, 2017

rmconsole-wf added Merge Requirements Met and removed Merge Requirements Unmet labels Apr 14, 2017

update unload demo page

98151cc

rmconsole-wf added Merge Requirements Unmet and removed Merge Requirements Met labels Apr 14, 2017

rmconsole-wf added Merge Requirements Met and removed Merge Requirements Unmet labels Apr 14, 2017

clarified docstring for unloadGroup

e16ffa0

rmconsole-wf added Merge Requirements Unmet and removed Merge Requirements Met labels Apr 14, 2017

lame. whitespace.

eceb397

rmconsole-wf added Merge Requirements Met and removed Merge Requirements Unmet labels Apr 14, 2017

rmconsole-wf added the RM Ready label Apr 14, 2017

rmconsole-wf merged commit 4065525 into master Apr 14, 2017

rmconsole-wf deleted the fix_unloading branch April 14, 2017 21:09

rmconsole-wf mentioned this pull request Apr 14, 2017

HY-4693 Release font_face_observer 2.0.0 #16

Merged

HY-4737 Fix unloading, refactor and cleanup #15

HY-4737 Fix unloading, refactor and cleanup #15

Conversation

robbecker-wf commented Apr 10, 2017 • edited Loading

Problem

Solution

+10 instructions

aviary2-wf commented Apr 10, 2017

Raven

rmconsole-wf commented Apr 10, 2017 • edited Loading

General Information

Additional Information

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomconnell-wf Apr 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanburke-wf commented Apr 14, 2017

semveraudit-wf commented Apr 14, 2017

seanburke-wf commented Apr 14, 2017

semveraudit-wf commented Apr 14, 2017

seanburke-wf commented Apr 14, 2017

semveraudit-wf commented Apr 14, 2017

stephenbush-wf commented Apr 14, 2017 • edited Loading

seanburke-wf commented Apr 14, 2017

seanburke-wf commented Apr 14, 2017

rmconsole-wf commented Apr 14, 2017

robbecker-wf commented Apr 10, 2017 •

edited

Loading

rmconsole-wf commented Apr 10, 2017 •

edited

Loading

tomconnell-wf Apr 14, 2017 •

edited

Loading

stephenbush-wf commented Apr 14, 2017 •

edited

Loading