Closed Bug 494372 Opened 12 years ago Closed 11 years ago

when views are notified skip static content

Categories

(Toolkit :: Places, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: mak, Assigned: mak)

References

Details

(Keywords: perf, Whiteboard: [TSnappiness])

Attachments

(1 file, 5 obsolete files)

Attached patch patch v1.0 (obsolete) — Splinter Review
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.
Attachment #379126 - Flags: review?(dietrich)
Keywords: perf
Whiteboard: [TSnappiness]
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 ?
hm, no it can't and you're right, so that max is useless. nice catch, will fix later.
Attachment #379126 - Flags: review?(dietrich)
Attached patch patch v1.1 (obsolete) — Splinter Review
fix the above comment
fix a missing else (probably we won't arrive there but better being paranoid)
Attachment #379126 - Attachment is obsolete: true
Attachment #379560 - Flags: review?(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.
Attachment #379560 - Flags: review?(dietrich) → review+
Flags: wanted1.9.1?
Flags: wanted1.9.1? → wanted1.9.1+
(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.
Flags: in-testsuite?
Whiteboard: [TSnappiness] → [TSnappiness][needs tests]
Attached patch test (obsolete) — Splinter Review
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.
Attachment #382420 - Flags: review?(dietrich)
Blocks: 456492
Whiteboard: [TSnappiness][needs tests] → [TSnappiness][needs review dietrich]
Attached patch test v1.1 (obsolete) — Splinter Review
added queries to test.
Attachment #382420 - Attachment is obsolete: true
Attachment #383750 - Flags: review?(dietrich)
Attachment #382420 - Flags: review?(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
Attachment #383750 - Flags: review?(dietrich) → review+
Whiteboard: [TSnappiness][needs review dietrich] → [TSnappiness]
(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.
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.
Attached patch merged patch v1.0 (obsolete) — Splinter Review
patch + test + fixed a casing issue that was creating problems on Linux. Still need to investigate OS X failures.
Attachment #379560 - Attachment is obsolete: true
Attachment #383750 - Attachment is obsolete: true
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.
Attachment #384833 - Attachment is obsolete: true
http://hg.mozilla.org/mozilla-central/rev/0a469e967395
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: in-testsuite? → in-testsuite+
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.