Closed
Bug 1032932
Opened 10 years ago
Closed 10 years ago
2.78% Win7 TResize regression from fx-team revision 920154597e3b on June 27th
Categories
(Firefox :: Toolbars and Customization, defect)
Tracking
()
RESOLVED
FIXED
Firefox 33
People
(Reporter: jmaher, Assigned: scaraveo)
References
Details
(Keywords: perf, regression, Whiteboard: [talos_regression])
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
Gijs
:
review+
lmandel
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
This is a small tresize regression that you can see on the graph server:
http://graphs.mozilla.org/graph.html#tests=[[254,131,25],[254,132,25],[254,94,25]]&sel=1403622101603,1404226901603&displayrange=7&datatype=running
I did some retriggers:
https://tbpl.mozilla.org/?tree=Fx-Team&jobname=Windows%207%2032-bit%20fx-team%20talos%20chromez&fromchange=38b404a28e4e&tochange=570982344606
It looks like this revision:
https://hg.mozilla.org/integration/fx-team/rev/920154597e3b
caused us to go from ~22.1 -> ~22.7
Reporter | ||
Comment 1•10 years ago
|
||
Shane, can you take a look at this. Luckily this is only Win7 and a small regression, but if there is something simple we can do lets try it out.
Flags: needinfo?(mixedpuppy)
Comment 2•10 years ago
|
||
2.10 <toolbarbutton id="social-share-button"
2.11 class="toolbarbutton-1 chromeclass-toolbar-additional"
2.12 - hidden="true"
Ugh. I should have caught this in review. I thought this node was in the palette. It being on the toolbar means we shouldn't unhide it by default, I don't think. :-\
Comment 3•10 years ago
|
||
This is a try with some small changes so the button can remain hidden on startup. Lets see if that addresses the regression.
https://tbpl.mozilla.org/?tree=Try&rev=738175e3308b
Attachment #8449531 -
Flags: review?(gijskruitbosch+bugs)
Flags: needinfo?(mixedpuppy)
Comment 4•10 years ago
|
||
The patch that caused this just got pushed to Aurora, so placing the necessary tracking flags on this so we make sure to uplift this patch to Aurora32 as well.
status-firefox32:
--- → affected
status-firefox33:
--- → affected
tracking-firefox32:
--- → ?
tracking-firefox33:
--- → ?
Comment 5•10 years ago
|
||
Comment on attachment 8449531 [details] [diff] [review]
fix talos regression with share button
Review of attachment 8449531 [details] [diff] [review]:
-----------------------------------------------------------------
::: browser/base/content/browser-social.js
@@ +530,5 @@
>
> get shareButton() {
> // web-panels (bookmark/sidebar) don't include customizableui, so
> // nsContextMenu fails when accessing shareButton, breaking
> // browser_bug409481.js.
Doesn't this mean you need to fix up nsContextMenu as well? And, from looking at mxr, MozSocialAPI ?
For better or for worse, this is a public API, and changing its return type might have unintended consequences. :-\
A safer patch (for uplift as well) would just change the update method to use CUI.
Attachment #8449531 -
Flags: review?(gijskruitbosch+bugs)
Comment 6•10 years ago
|
||
simpler approach from Gijs.
https://tbpl.mozilla.org/?tree=Try&rev=4439b0f34cd0
Attachment #8449531 -
Attachment is obsolete: true
Attachment #8449607 -
Flags: review?(gijskruitbosch+bugs)
Updated•10 years ago
|
Attachment #8449607 -
Flags: review?(gijskruitbosch+bugs) → review+
Updated•10 years ago
|
Comment 7•10 years ago
|
||
Reporter | ||
Comment 8•10 years ago
|
||
Thanks guys for getting this fixed!
Comment 9•10 years ago
|
||
Assignee: nobody → scaraveo
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 33
Comment 10•10 years ago
|
||
Comment on attachment 8449607 [details] [diff] [review]
fix talos regression with share button
Approval Request Comment
[Feature/regressing bug #]: 1014254
[User impact if declined]: talos regression due to share button being unecessarily visible
[Describe test coverage new/current, TBPL]: current
[Risks and why]: low
[String/UUID change made/needed]: none
Attachment #8449607 -
Flags: approval-mozilla-aurora?
Comment 11•10 years ago
|
||
another note: bug 1014254 was uplifted to aurora, but this fix has not
Comment 12•10 years ago
|
||
Not only is there a talos regression if we don't uplift to aurora, it also causes a confusing UX. The share button shows up when you have no providers installed and it is perma-disabled. So, that's kind of weird. I would add that as another reason we should uplift this.
Comment 13•10 years ago
|
||
Comment on attachment 8449607 [details] [diff] [review]
fix talos regression with share button
Aurora+
Attachment #8449607 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 14•10 years ago
|
||
Comment 15•10 years ago
|
||
I accidentally backed this out when I meant to backout a different bug for bustage. Re-landed.
https://hg.mozilla.org/releases/mozilla-aurora/rev/f4397dde382d
You need to log in
before you can comment on or make changes to this bug.
Description
•