Closed Bug 1028872 Opened 10 years ago Closed 10 years ago

Intermittent PGO browser_938980_navbar_collapsed.js | The bookmarksToolbar should have height=0 after reset - Got 6.899993896484375

Categories

(Core :: General, defect)

Product:
Core
Component:
General
Platform:
x86
Windows 8
Type:
defect
Priority:
Not set
Severity:
normal
Points:
1

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla33
Iteration:
33.2
Tracking Flags:
Tracking Status
firefox31 --- unaffected
firefox32 --- unaffected
firefox33 --- fixed
firefox-esr24 --- unaffected

People

(Reporter: cbook, Assigned: Gijs)

References

(
URL
)

Details

(Keywords: intermittent-failure)

Attachments

(1 file, 1 obsolete file)

listen for bookmarks toolbar transition,
(deleted), patch
dao
: review+
Details | Diff | Splinter Review
WINNT 6.2 fx-team pgo test mochitest-browser-chrome-1 on 2014-06-22 13:43:51 PDT for push 61c817d378b2 slave: t-w864-ix-128 https://tbpl.mozilla.org/php/getParsedLog.php?id=42233244&tree=Fx-Team TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/customizableui/test/browser_938980_navbar_collapsed.js | The bookmarksToolbar should have height=0 after reset - Got 6.899993896484375 Poiru: can this be related to bug 1016240 , seems this is pgo only somehow
Flags: needinfo?(birunthan)
I would be very surprised if there were any relationship between this and bug 1016240, especially considering that this is PGO-only. It might be caused by a timing differece between PGO and non-PGO builds. Based on recent changes to browser/components/customizableui/, I think bug 1011392, bug 979207, or bug 994117 might be more likely candidates. That said, if you want to try backing out bug 1016240, let me know if there are any conflicts.
Flags: needinfo?(birunthan)
This is because the toolbars are now animating (bug 867317) and on PGO builds they are sometimes still animating by the time the test code gets invoked. We should wait for the animation/transition to finish. Marco, can you add this to this iteration?
Assignee: nobody → gijskruitbosch+bugs
Blocks: 867317
Status: NEW → ASSIGNED
Iteration: --- → 33.2
Points: --- → 1
Flags: needinfo?(mmucci)
Attached patch listen for bookmarks toolbar transition, (obsolete) (deleted) — — Splinter Review
This should fix the issue, I believe.
Attachment #8447129 - Flags: review?(dao)
Blocks: 1028866
Added to Iteration 33.2.
QA Whiteboard: [qa?]
Flags: needinfo?(mmucci)
Comment on attachment 8447129 [details] [diff] [review] listen for bookmarks toolbar transition, > isnot(bookmarksToolbar.getBoundingClientRect().height, 0, "The bookmarksToolbar should be visible before reset"); > isnot(navbar.getBoundingClientRect().height, 0, "The navbar should be visible before reset"); > isnot(tabsToolbar.getBoundingClientRect().height, 0, "TabsToolbar should have non-zero height"); > is(bookmarksToolbar.getBoundingClientRect().height, 0, "The bookmarksToolbar should have height=0 after reset"); > isnot(tabsToolbar.getBoundingClientRect().height, 0, "TabsToolbar should have non-zero height"); > isnot(navbar.getBoundingClientRect().height, 0, "The navbar should still be visible after reset"); I think we should just check toolbar.collapsed here. It's clear that themes can override that, but this test doesn't need to be concerned with this.
Attachment #8447129 - Flags: review?(dao)
OK.
Attachment #8447179 - Flags: review?(dao)
Attachment #8447129 - Attachment is obsolete: true
(I left the menubar checks as-is because they use autohide=true so checking for collapsed doesn't work, and because they're not impacted by this failure)
Comment on attachment 8447179 [details] [diff] [review] listen for bookmarks toolbar transition, > setToolbarVisibility(navbar, false); >+ ok(navbar.collapsed, "navbar should be collapsed"); Testing this is kind of pointless, since we don't allow users to hide the navigation toolbar. It's arguably a bug that setToolbarVisibility supports this.
Attachment #8447179 - Flags: review?(dao) → review+
(In reply to Dão Gottwald [:dao] from comment #51) > Comment on attachment 8447179 [details] [diff] [review] > listen for bookmarks toolbar transition, > > > setToolbarVisibility(navbar, false); > > >+ ok(navbar.collapsed, "navbar should be collapsed"); > > Testing this is kind of pointless, since we don't allow users to hide the > navigation toolbar. It's arguably a bug that setToolbarVisibility supports > this. I believe this was added to ensure that when people migrate from older versions where we did allow this, they don't end up in a permanently broken state, so we made CUI.reset() cope and added a test for it. I interpreted your comment as being OK with leaving this for now, but rs=me to remove this part of the test in a followup or a new bug; if you want to update setToolbarVisibility, please file a new bug for that. remote: https://hg.mozilla.org/integration/fx-team/rev/f2e8982acbf0
QA Whiteboard: [qa?] → [qa-]
https://hg.mozilla.org/mozilla-central/rev/f2e8982acbf0
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Flags: firefox-backlog+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: