HY-4649 track loaded fonts and add methods to unload fonts #10

robbecker-wf · 2017-03-24T20:16:04Z

Problem

There is no way to unload fonts once they are loaded

Solution

Track loaded fonts by key and group and provide a way to unload a single font by unique key or multiple fonts by group.
Bonus changes: each font is loaded with a separate style tag with attributes for key, group, and the number of uses.

Areas of Impact

Font loading and unloading.

Test Overview (+10 Instructions)

pub get and pub serve test
open http://localhost:8080/unload.html
Open dev tools and follow instructions.
expand the head element watch style elements be added and removed

Versioning

Major
- This change removes part of the public API or modifies it in an incompatible manner
  - Justification for not being backwards-compatible:
Minor
- This change adds something to the public API in a backwards-compatible manner
- This change deprecates a part of the public API
Patch
- This change does not affect the public API
- This change fixes existing incorrect behavior
- This change only affects tests (and not any exported test API, like test ids)

aviary2-wf · 2017-03-24T20:16:07Z

Raven

Number of Findings: 0

rmconsole-wf · 2017-03-24T20:16:17Z

General Information

Ticket(s):

HY-4649 | remove

Code Review(s): #10

Additional Information

Reviewers: patkujawa-wf, tomconnell-wf
Watchlist Notifications: None

	When this pull is merged I will use the following information:
	Version: font_face_observer 1.0.2
	Release Ticket(s): HY-4420

Last updated on Wednesday, March 29 12:07 PM CST

robbecker-wf · 2017-03-27T16:09:52Z

lib/font_face_observer.dart

@@ -23,7 +23,6 @@ const int DEFAULT_TIMEOUT = 3000;
 const String DEFAULT_TEST_STRING = 'BESbswy';
 const String _NORMAL = 'normal';
 const int _NATIVE_FONT_LOADING_CHECK_INTERVAL = 50;
-const String _FONT_FACE_CSS_ID = 'FONT_FACE_CSS';


constant for css class names moved to ruler.dart

patkujawa-wf · 2017-03-27T17:33:34Z

lib/font_face_observer.dart

+  int get uses => _uses;
+  void set uses(int new_uses) {
+    _uses = new_uses;
+    element.attributes['uses'] = _uses.toString();


Could be data-uses, but meh.

I kinda like the data attributes especially since this isn't a custom element. I'll revamp to use that.

robbecker-wf · 2017-03-27T17:52:51Z

Ok, updated to use data attributes. Dart makes it super easy to read/write data attributes with the dataset Map on Element. so #winning.

patkujawa-wf · 2017-03-27T17:58:16Z

test/font_face_observer_test.dart

@@ -70,6 +71,55 @@ main() {
      expect(result.isLoaded, isTrue);
    });

+    test('should not leave temp DOM nodes after detecting', () async {


Which part of this test is "detecting"? ffo.load? isLoaded?

.load loads the fonts and waits for it to be loaded. Should I change the wording in the test desc to "load" ?

patkujawa-wf · 2017-03-27T18:01:22Z

test/font_face_observer_test.dart

+      expect(styleElement.dataset['uses'],'1');
+
+
+    });


Might be nice to unload multiple times and see if count goes negative.

It will be removed once it is less than 1 and is covered by the test test('should unload a font by key', () async {

patkujawa-wf · 2017-03-27T18:16:02Z

lib/font_face_observer.dart

+      var loadedFont = _loadedFonts[k];
+      if (loadedFont.group == group ||
+          (group == "" && loadedFont.group == null)) {
+        keysToRemove.add(k);


It's very surprising that passing null into this function will unload all fonts that were created without specifying a group. We chatted offline about this.

I updated to require a group, it uses a default value if you specify none.

patkujawa-wf · 2017-03-27T18:22:23Z

lib/font_face_observer.dart

+    return loadedFont != null && loadedFont.uses > 0;
+  }
+
+  static int unloadGroup(String group) {


docstrings pls. Specifically, what should the return value represent? And what are invalid inputs for group?

patkujawa-wf · 2017-03-27T18:26:37Z

lib/font_face_observer.dart

+      FontLoadResult r = await _result.future;
+      if (r.isLoaded) {
+        _LoadedFont loadedFont = _getLoadedFont(url);
+        loadedFont.uses++;
      }


So, what if the result isn't loaded? Should this add a then to increment uses when it is loaded?

patkujawa-wf · 2017-03-27T18:27:31Z

lib/font_face_observer.dart

  String get testString => _testString;
+
+  _LoadedFont _getLoadedFont(String url) {
+    var styleElement;


Would be nice to have this typed, even as just Object or dynamic. EDIT: Why is it even outside the scope of the else?

patkujawa-wf · 2017-03-27T18:29:30Z

lib/font_face_observer.dart


  FontFaceObserver(String this.family,
      {String this.style: _NORMAL,
      String this.weight: _NORMAL,
      String this.stretch: _NORMAL,
      String testString: DEFAULT_TEST_STRING,
      int this.timeout: DEFAULT_TIMEOUT,
-      bool this.useSimulatedLoadEvents: false}) {
+      bool this.useSimulatedLoadEvents: false,
+      String this.group: ""}) {


As we discussed offline, use a default group and then add validation if group is customized that it must not be null or empty.

patkujawa-wf

Let's clean up grouping, plus some docstrings, then g2g.

patkujawa-wf · 2017-03-27T22:52:31Z

lib/font_face_observer.dart

  bool get isLoaded {
    var loadedFont = _loadedFonts[key];
    return loadedFont != null && loadedFont.uses > 0;
  }

+  /// A list of font keys for all currently loaded fonts
+  static List<String> get loadedFontKeys {
+    return _loadedFonts.keys.toList(growable: false);


Probably better to return this as a Set, because won't users most likely do a membership check? Or, asking another way, does the order really matter? If not, a set would be a more efficient way to check if a font is in the list of loaded keys.

patkujawa-wf · 2017-03-27T22:55:38Z

lib/font_face_observer.dart

+        loadedGroups.add(loadedFont.group);
+      }
+    });
+    return loadedGroups;


If this function returned a Set, it would be easier to code (no need to check if it already contains), plus it would guarantee that the groups are unique (and thus no need for the docstring addendum).

patkujawa-wf · 2017-03-27T23:02:38Z

lib/font_face_observer.dart

+  /// A list of font keys for all currently loaded fonts
+  static List<String> get loadedFontKeys {
+    return _loadedFonts.keys.toList(growable: false);
+  }


This should return a Set too

patkujawa-wf · 2017-03-27T23:05:02Z

+1 CR with suggestions to use a Set

patkujawa-wf · 2017-03-27T23:14:28Z

+1 still

patkujawa-wf · 2017-03-28T17:29:24Z

lib/font_face_observer.dart

  }

  /// A list of groups that the currently loaded fonts are in
  /// There will not be duplicate group entries if there are multiple fonts
  /// in the same group.
  static Iterable<String> get loadedGroups {
-    List<String> loadedGroups = [];
+    Set<String> loadedGroups = new Set<String>();
    _loadedFonts.keys.forEach((k) {
      var loadedFont = _loadedFonts[k];
      if (!loadedGroups.contains(loadedFont.group)) {


No need to do this check now that it's a Set

patkujawa-wf · 2017-03-28T20:13:04Z

+1 CR

tomconnell-wf · 2017-03-29T17:03:39Z

+10

Style elements get added and removed. Memory and nodes go down a little bit, but not all the way back down. (There is still one font that wouldn't unload.)

tomconnell-wf · 2017-03-29T17:05:07Z

QA +1

Merging into master.

tomconnell-wf · 2017-03-29T17:07:35Z

@Workiva/release-management-pp - Rosie, will you automerge?

rmconsole-wf · 2017-03-29T17:07:52Z

+1 from RM

tomconnell-wf · 2017-03-29T17:09:53Z

run_merge_script

track, and add methods to unload fonts

5399ce2

robbecker-wf added 5 commits March 24, 2017 16:16

give classes to temp elements, add unload example

9ddd413

address TODO

e49981c

add unit tests, fix dom leak

adf6a51

ddev format

b28b721

be consistent with _ffo for classnames

fbc9647

robbecker-wf commented Mar 27, 2017

View reviewed changes

add comments, fix typo

e32e7fc

robbecker-wf changed the title ~~WIP track, and add methods to unload fonts~~ track loaded fonts and add methods to unload Mar 27, 2017

rmconsole2-wf changed the title ~~track loaded fonts and add methods to unload~~ HY-4649 WIP track, and add methods to unload fonts Mar 27, 2017

patkujawa-wf changed the title ~~HY-4649 WIP track, and add methods to unload fonts~~ HY-4649 track, and add methods to unload fonts Mar 27, 2017

patkujawa-wf reviewed Mar 27, 2017

View reviewed changes

use data attributes

ace4133

robbecker-wf changed the title ~~HY-4649 track, and add methods to unload fonts~~ HY-4649 track loaded fonts and add methods to unload fonts Mar 27, 2017

patkujawa-wf reviewed Mar 27, 2017

View reviewed changes

patkujawa-wf requested changes Mar 27, 2017

View reviewed changes

robbecker-wf added 4 commits March 27, 2017 12:52

everything must have a group or use the default

20a240d

support updating the group

6d1d470

much cleanup

a880cb9

format

d22ab0d

patkujawa-wf reviewed Mar 27, 2017

View reviewed changes

patkujawa-wf approved these changes Mar 27, 2017

View reviewed changes

rmconsole-wf added the Merge Requirements Met label Mar 27, 2017

iterable

9d16684

rmconsole-wf added Merge Requirements Unmet and removed Merge Requirements Met labels Mar 27, 2017

thanks unit tests!

b9597cb

patkujawa-wf reviewed Mar 28, 2017

View reviewed changes

properties -> getters cause they do work

d827e4a

rmconsole-wf added Merge Requirements Met and removed Merge Requirements Unmet labels Mar 28, 2017

rmconsole-wf added the RM Ready label Mar 29, 2017

rmconsole-wf merged commit c3f376e into master Mar 29, 2017

rmconsole-wf deleted the unload_fonts branch March 29, 2017 17:07

rmconsole-wf mentioned this pull request Mar 29, 2017

Release font_face_observer 1.0.2 #11

Closed

rmconsole2-wf mentioned this pull request Mar 29, 2017

HY-4420 RELEASE font_face_observer 1.1.0 #12

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HY-4649 track loaded fonts and add methods to unload fonts #10

HY-4649 track loaded fonts and add methods to unload fonts #10

robbecker-wf commented Mar 24, 2017 •

edited by tomconnell-wf

Loading

aviary2-wf commented Mar 24, 2017

rmconsole-wf commented Mar 24, 2017 •

edited

Loading

robbecker-wf Mar 27, 2017

patkujawa-wf Mar 27, 2017

robbecker-wf Mar 27, 2017 •

edited

Loading

robbecker-wf commented Mar 27, 2017

patkujawa-wf Mar 27, 2017

robbecker-wf Mar 27, 2017

patkujawa-wf Mar 27, 2017

robbecker-wf Mar 27, 2017

patkujawa-wf Mar 27, 2017

robbecker-wf Mar 27, 2017

patkujawa-wf Mar 27, 2017

patkujawa-wf Mar 27, 2017

patkujawa-wf Mar 27, 2017 •

edited

Loading

patkujawa-wf Mar 27, 2017

patkujawa-wf left a comment

patkujawa-wf Mar 27, 2017

patkujawa-wf Mar 27, 2017

patkujawa-wf Mar 27, 2017

patkujawa-wf commented Mar 27, 2017

patkujawa-wf commented Mar 27, 2017

patkujawa-wf Mar 28, 2017

patkujawa-wf commented Mar 28, 2017

tomconnell-wf commented Mar 29, 2017

tomconnell-wf commented Mar 29, 2017

tomconnell-wf commented Mar 29, 2017

rmconsole-wf commented Mar 29, 2017

tomconnell-wf commented Mar 29, 2017

HY-4649 track loaded fonts and add methods to unload fonts #10

HY-4649 track loaded fonts and add methods to unload fonts #10

Conversation

robbecker-wf commented Mar 24, 2017 • edited by tomconnell-wf Loading

Problem

Solution

Areas of Impact

Test Overview (+10 Instructions)

aviary2-wf commented Mar 24, 2017

Raven

rmconsole-wf commented Mar 24, 2017 • edited Loading

General Information

Additional Information

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robbecker-wf Mar 27, 2017 • edited Loading

Choose a reason for hiding this comment

robbecker-wf commented Mar 27, 2017

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

patkujawa-wf Mar 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patkujawa-wf left a 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

patkujawa-wf commented Mar 27, 2017

patkujawa-wf commented Mar 27, 2017

Choose a reason for hiding this comment

patkujawa-wf commented Mar 28, 2017

tomconnell-wf commented Mar 29, 2017

+10

tomconnell-wf commented Mar 29, 2017

QA +1

tomconnell-wf commented Mar 29, 2017

rmconsole-wf commented Mar 29, 2017

tomconnell-wf commented Mar 29, 2017

robbecker-wf commented Mar 24, 2017 •

edited by tomconnell-wf

Loading

rmconsole-wf commented Mar 24, 2017 •

edited

Loading

robbecker-wf Mar 27, 2017 •

edited

Loading

patkujawa-wf Mar 27, 2017 •

edited

Loading