494372 - when views are notified skip static content

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch v1.0

When searching for a node in views we should skip static content, this should have a minimal speed gain, but mostly fixes javascript strict warnings that happen much frequently along the UI.

also removes a call to not existant _forwardToChildView method.

Comment 1

[Dietrich Ayala (:dietrich)]

Comment on attachment 379126 [details] [diff] [review]
patch v1.0

>           var children = popup.childNodes;
>-          for (var i = 0; i < children.length; i++) {
>+          var start = Math.max(popup._startMarker + 1, 0);
>+          for (var i = start; i < children.length; i++) {

i don't understand this. _startMarker can be < -1 ?

Comment 2

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

hm, no it can't and you're right, so that max is useless. nice catch, will fix later.

Comment 3

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch v1.1

fix the above comment
fix a missing else (probably we won't arrive there but better being paranoid)

Comment 4

[Dietrich Ayala (:dietrich)]

Comment on attachment 379560 [details] [diff] [review]
patch v1.1


> 
>         itemChanged: function PMV_itemChanged(aNode) {
>           // this check can be removed once we fix bug #382397
>           var parentNode = aNode.parent;
>           if (!parentNode)
>             return;
> 
>+          if (PlacesUtils.nodeIsSeparator(aNode)) {
>+            // nothing to do when a separator changes
>+            return;
>+          }

in that above check, bug 382397 has been resolved invalid.. can that check go away now? are there others like it? can file a followup about this if you want.

r=me with tests: changes to the views are very regression prone, so if we don't already have some tests that cover basic live update scenarios w/ these views, then we should add them with these changes.

Comment 5

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to comment #4)
> (From update of attachment 379560 [details] [diff] [review])
> in that above check, bug 382397 has been resolved invalid.. can that check go
> away now? are there others like it? can file a followup about this if you want.

now that i read that code again i think i'll reopen that bug. mParent is valid but in the view we use mParent->mParent. so Reopening.

Comment 6

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: test

this is a bit cumbersome, but it's hard to test views live update with some common code. This tests live update when adding, moving and removing bookmarks. Can eventually be expanded in future.

Comment 7

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: test v1.1

added queries to test.

Comment 8

[Dietrich Ayala (:dietrich)]

Comment on attachment 383750 [details] [diff] [review]
test v1.1


>+function test() {
>+  dump("Starting test browser_views_liveupdate.js\n");
>+  waitForExplicitFinish();
>+
>+  // Sanity checks.
>+  ok(PlacesUtils, "PlacesUtils in context");
>+  ok(PlacesUIUtils, "PlacesUIUtils in context");
>+
>+  // Open bookmarks menu.
>+  var menu = document.getElementById("bookmarksMenu");
>+  menu.open = true;
>+
>+  // Open bookmarks sidebar.
>+  var sidebar = document.getElementById("sidebar");
>+  sidebar.addEventListener("load", function() {
>+    sidebar.removeEventListener("load", arguments.callee, true);
>+    // Need to executeSoon since the tree is initialized on sidebar load.
>+    executeSoon(startTest);
>+  }, true);
>+  toggleSidebar("viewBookmarksSidebar", true);
>+}

wouldn't opening the sidebar cause the menu to be closed? (just want to ensure that some tests aren't "passing" because they're not actually being run)

r=me otherwise

Comment 9

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to comment #8)
> wouldn't opening the sidebar cause the menu to be closed? (just want to ensure
> that some tests aren't "passing" because they're not actually being run)

No, there is no focus change, and it actually does not cause that, i see the menu open and populating during test run. Maybe i should first push to the tryserver to be sure this does not have strange behaviors on non Windows boxes.

Comment 10

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

try server shows issues on OS X (menu tests are failing, could be the issue you were talking about, or the fact menu is native). i should maybe disable menu tests on Mac, will check.

Comment 11

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: merged patch v1.0

patch + test + fixed a casing issue that was creating problems on Linux. Still need to investigate OS X failures.

Comment 12

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: merged patch v1.1

Disabled menu tests on OS X, fixed menus opening on Mac and Linux.
I pushed this to the tryserver and did not notice any related failure.

Comment 13

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

http://hg.mozilla.org/mozilla-central/rev/0a469e967395