644641 - Fix perma-oranges browser-chrome tests on desktop

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch

The patch does not fix the forms's tests failures (this will be handle by bug 641825) but fix the perma-orange we currently have on desktop.

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 521532 [details] [diff] [review]
Patch

>diff --git a/chrome/content/browser.xul b/chrome/content/browser.xul

>-          <richlistitem class="context-command" id="context-bookmark-link" type="link" onclick="ContextCommands.bookmarkLink();">
>+          <richlistitem class="context-command" id="context-bookmark-link" type="link-bookmarkable" onclick="ContextCommands.bookmarkLink();">

>diff --git a/chrome/content/content.js b/chrome/content/content.js

>-ContextHandler.registerType("link", function(aState, aElement) {
>+ContextHandler.registerType("link-bookmarkable", function(aState, aElement) {
>   return !!aState.linkURL;

I don't understand the need for this change. And after talking to you on IRC I understand it better. Please remove this change. The best type we can use for the case where "!!aState.linkURL" is "link"

"link-bookmarkable" is too specific to the one use case we have. If we find a reason to make bookmarking not work on any link, then we can add "link-bookmarkable" and we will filter the types of URLs allowed to be bookmarked.

r+, but remove "link-bookmarkable" from code, XUL and tests.

Comment 2

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

http://hg.mozilla.org/mobile-browser/rev/295f4fb3f47f

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

You forgot to add back registering the "link" type:
http://hg.mozilla.org/mobile-browser/rev/295f4fb3f47f#l2.12

We need that, right?

Comment 4

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to comment #3)
> You forgot to add back registering the "link" type:
> http://hg.mozilla.org/mobile-browser/rev/295f4fb3f47f#l2.12
> 
> We need that, right?

We don't, "link" is already added by http://hg.mozilla.org/mobile-browser/file/5d299232e62a/chrome/content/content.js#l824

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #4)
> (In reply to comment #3)
> > You forgot to add back registering the "link" type:
> > http://hg.mozilla.org/mobile-browser/rev/295f4fb3f47f#l2.12
> > 
> > We need that, right?
> 
> We don't, "link" is already added by
> http://hg.mozilla.org/mobile-browser/file/5d299232e62a/chrome/content/content.js#l824

Great. Thanks for checking

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

Regression Tp4 (Private Bytes) increase 7.33% on Nokia n900 mobile
---------------------------------------------------------------------
    Previous: avg 241913766.667 stddev 1434163.428 of 30 runs up to revision 3fa9692ebcb4
    New     : avg 259642800.000 stddev 1175958.205 of 5 runs since revision 295f4fb3f47f
    Change  : +17729033.333 (7.33% / z=12.362)
    Graph   : http://mzl.la/hp1hVV

Changeset range: http://hg.mozilla.org/mobile-browser/pushloghtml?fromchange=3fa9692ebcb4&tochange=295f4fb3f47f

Changesets:
  * http://hg.mozilla.org/mobile-browser/rev/295f4fb3f47f
    : Vivien Nicolas <[e-mail]> - Bug 644641: Fix perma-oranges browser-chrome tests on desktop [r=mfinkle]
    : http://bugzilla.mozilla.org/show_bug.cgi?id=644641

I still need to see if any mozilla-central changes could be to blame, but I did notice something that could be a problem: We no longer cache the mobileRoot, so we call some Places code on every "star" button press. We have speed issues with starring.

Maybe we could change the new <property> to use a <field> and do something like:

 <property name="_mobileRoot">null</field>
 <property name="mobileRoot" onget="if (!this._mobileRoot) this._mobileRoot =  PlacesUtils.annotations.getItemsWithAnnotation('mobile/bookmarksRoot', {})[0]; return this._mobileRoot;"/>

That should add caching back to the property, only after a valid result is available. I would also switch to a <getter> instead of the onget=""

Comment 7

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to comment #6)
> Regression Tp4 (Private Bytes) increase 7.33% on Nokia n900 mobile
> ---------------------------------------------------------------------
>     Previous: avg 241913766.667 stddev 1434163.428 of 30 runs up to revision
> 3fa9692ebcb4
>     New     : avg 259642800.000 stddev 1175958.205 of 5 runs since revision
> 295f4fb3f47f
>     Change  : +17729033.333 (7.33% / z=12.362)
>     Graph   : http://mzl.la/hp1hVV
> 
> Changeset range:
> http://hg.mozilla.org/mobile-browser/pushloghtml?fromchange=3fa9692ebcb4&tochange=295f4fb3f47f
> 
> Changesets:
>   * http://hg.mozilla.org/mobile-browser/rev/295f4fb3f47f
>     : Vivien Nicolas <[e-mail]> - Bug 644641: Fix perma-oranges
> browser-chrome tests on desktop [r=mfinkle]
>     : http://bugzilla.mozilla.org/show_bug.cgi?id=644641
> 
> I still need to see if any mozilla-central changes could be to blame, but I did
> notice something that could be a problem: We no longer cache the mobileRoot, so
> we call some Places code on every "star" button press. We have speed issues
> with starring.
> 
> Maybe we could change the new <property> to use a <field> and do something
> like:
> 
>  <property name="_mobileRoot">null</field>
>  <property name="mobileRoot" onget="if (!this._mobileRoot) this._mobileRoot = 
> PlacesUtils.annotations.getItemsWithAnnotation('mobile/bookmarksRoot', {})[0];
> return this._mobileRoot;"/>
> 
> That should add caching back to the property, only after a valid result is
> available. I would also switch to a <getter> instead of the onget=""

Sounds a good plan since this won't hurt even if that's not the root cause, I will attach a followup

Comment 8

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch - followup

Comment 9

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

pushed the followup: http://hg.mozilla.org/mobile-browser/rev/c358e083b0ad