794443 - Port | Bug 781588 - URLBarSetURI takes about 63ms to SeaMonkey

Status: RESOLVED FIXED

(SeaMonkey :: Tabbed Browser, defect)

Description

[Phoenix]

Attachment: urlbarBindings patch

"Blind" patch for not-shared parts of that bug

Comment 1

[[e-mail]]

Comment on attachment 664919 [details] [diff] [review]
urlbarBindings patch

>+              if (!domain.contains(baseDomain)) {
[I wonder whether they meant endsWith.]

>+                let IDNService = Cc["@mozilla.org/network/idn-service;1"]
>+                                 .getService(Ci.nsIIDNService);
>+                baseDomain = IDNService.convertACEtoUTF8(baseDomain);
We don't have Cc/Ci so you'll need to write that out in full.

Comment 2

[Phoenix]

Attachment: Patch v2

Fixed review comment

Comment 3

[Philip Chee]

> +              if (!domain.contains(baseDomain)) {

Neil says: [I wonder whether they meant endsWith.]
And the answer is yes they did.

> +                // getBaseDomainFromHost converts its resultant to ACE.
> +                let IDNService = Components.classes["@mozilla.org/network/idn-service;1"]
> +                                 .getService(Components.interfaces.nsIIDNService);
Line up the dots. Like. This.
let IDNService = Components.classes["@mozilla.org/network/idn-service;1"]
                           .getService(Components.interfaces.nsIIDNService);

Comment 4

[Phoenix]

Attachment: Patch v3

Fixed indentation and incorporate Bug 794520 change

Comment 5

[[e-mail]]

Comment on attachment 665306 [details] [diff] [review]
Patch v3

Bah, missed these first time:

>-              var baseDomain = Services.eTLD.getBaseDomainFromHost(domain);
>+              baseDomain = Services.eTLD.getBaseDomainFromHost(domain);
This change is wrong.

>-              if (baseDomain != domain) {
...
>+          if (baseDomain != domain) {
>+            subDomain = domain.slice(0, -baseDomain.length);
>+          }
Code needs to be moved back inside the try/catch.

r=me with those fixed.

Comment 6

[Phoenix]

Attachment: Patch v4

Comment 7

[Phoenix]

(In reply to [e-mail] from comment #5)
> Comment on attachment 665306 [details] [diff] [review]
> Patch v3
> 
> Bah, missed these first time:
> 
> >-              var baseDomain = Services.eTLD.getBaseDomainFromHost(domain);
> >+              baseDomain = Services.eTLD.getBaseDomainFromHost(domain);
> This change is wrong.
Heh, I thought that I revert this, but...

> >-              if (baseDomain != domain) {
> ...
> >+          if (baseDomain != domain) {
> >+            subDomain = domain.slice(0, -baseDomain.length);
> >+          }
> Code needs to be moved back inside the try/catch.
Done. So, they made another mistake or this difference in implementation?

> r=me with those fixed.
Just in case, check last time plz :)

Comment 8

[[e-mail]]

(In reply to Phoenix from comment #7)
> So, they made another mistake or this difference in implementation?
Both the "changes" are because of a subtle difference in implementation. Sorry about that.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/d8e9fe83bd2b

Thanks for the patch, Phoenix! One request - please make sure that future patches follow the guidelines below so that all the needed commit information is present in them. It makes life easier for those checking in on your behalf. Thanks!
https://developer.mozilla.org/en-US/docs/Creating_a_patch_that_can_be_checked_in

Comment 10

[[e-mail]]

Comment on attachment 665327 [details] [diff] [review]
Patch v4

Eek! The "s" of components vanished on line 129 :-(

Also the patch got checked in with DOS line endings. Oops.

Comment 11

[[e-mail]]

Pushed comm-central changeset 0226b8750d2a to address the above issues.