294673 - Starting Reporter from a blank page shouldn't prefill URL with "about:blank"

Status: RESOLVED FIXED

(Other Applications Graveyard :: Reporter, defect)

Description

[Jeff Walden [:Waldo]]

If you open up Reporter from a blank page, Reporter automatically uses
about:blank as the problem page.  This is wrong, because it's never the site
that's desired.  Additionally, submitting a report with that URL generates an
error for an invalid URL.  Instead, when a blank page is shown the URL field
should be left blank.

Comment 1

[Robert Accettura [:raccettura]]

Yea.

I've been meaning to get to this.  Is there a method for ensuring that only
proper url's are reported (no internal's or about:)?

It's not critical, as the server knows about this, and kicks those out anyway. 
But it's something we should fix.

Comment 2

[Jeff Walden [:Waldo]]

So, XULPlanet's got a nice list of all the schemes Mozilla uses:

http://xulplanet.com/references/xpcomref/group_Network.html#Protocols

I think the best bet is just to cherry-pick from the list and choose the ones it
makes sense to allow.

Comment 3

[Darin Fisher]

Why don't you just make the URL field editable?  And, I would special case
about:blank since the rest of the browser treats it as special.

Comment 4

[Robert Accettura [:raccettura]]

I'm ok with special casing about:blank.

Though I don't want the field to be open for edits.  The point of it being read
only is so people don't tamper with it. www.foo.tld/bar/baz!= www.foo.tld.  We
need percision.

Comment 5

[Robert Accettura [:raccettura]]

Attachment: Patch

Based on 264562

Comment 6

[Robert Accettura [:raccettura]]

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

The big question here is: should be totally silent like that?  Or will that
cause confusion?

Comment 7

[Jeff Walden [:Waldo]]

(In reply to comment #6)
> The big question here is: should be totally silent like that?  Or will that
> cause confusion?

I'd think the best way to do this would be to enable or disable the menu item
based on the current URI each time the menu containing the reporter item is opened.

Comment 8

[Robert Accettura [:raccettura]]

(In reply to comment #7)
> I'd think the best way to do this would be to enable or disable the menu item
> based on the current URI each time the menu containing the reporter item is
opened.

My question about that is how would that impact on performance?  Would it
calculate every url entered?  Or would it only render when you open the menu? 
And what about really really long url's?

Comment 9

[Jeff Walden [:Waldo]]

(In reply to comment #8)
> My question about that is how would that impact on performance?

How much performance is lost when you select a bit of text on a web page, put
focus somewhere, or copy something?  Every time you do so, all the Edit items
related to focus get updated this way (or, more correctly, the <command/>
elements for each gets enabled or disabled appropriately).

> Would it calculate every url entered?

You could have the items updated on each page load if you wanted, and I even
considered suggesting this be done that way.  My suggestion, however, does not
update the menu items every time you load a new page in the browser or switch to
a tab containing a different page.

> Or would it only render when you open the menu?

This suggestion would do it this way, yes.  At least one popular extension whose
behavior depends on the current page's URI does it this way.

> And what about really really long url's?

I'm not sure what you mean.  The size of the URI should have no impact on how
this would be implemented, and the speed difference between small and large URIs
should be negligible.  Besides, I'm pretty sure you'd get the URI the same way
you do now - any performance hit should already be in current code.

Comment 10

[Robert Accettura [:raccettura]]

(In reply to comment #9)
> > My question about that is how would that impact on performance?
> 
> How much performance is lost when you select a bit of text on a web page, put
> focus somewhere, or copy something?  Every time you do so, all the Edit items
> related to focus get updated this way (or, more correctly, the <command/>
> elements for each gets enabled or disabled appropriately).
> 
Is that chrome code?  Or deeper inside the beast?

> > Or would it only render when you open the menu?
> 
> This suggestion would do it this way, yes.  At least one popular extension whose
> behavior depends on the current page's URI does it this way.
> 
That's how it should be done.  I want reporter to be 0 impact.  Need to figure
out how that's done.

> > And what about really really long url's?
> 
> I'm not sure what you mean.  The size of the URI should have no impact on how
> this would be implemented, and the speed difference between small and large URIs
> should be negligible.  Besides, I'm pretty sure you'd get the URI the same way
> you do now - any performance hit should already be in current code.

well, in theory, indexOf() on a 3 char string is quicker than 3.02x10^23.

Comment 11

[Jeff Walden [:Waldo]]

(In reply to comment #10)
> Is that chrome code?  Or deeper inside the beast?

That's all in js chrome files.

> well, in theory, indexOf() on a 3 char string is quicker than 3.02x10^23.

Oops - didn't notice you were using indexOf().  You should be using
getBrowser().currentURI.schemeIs("foo") instead, which will be faster because
it's preconstructed in the currentURI XPConnect-wrapped nsIURI.

Comment 12

[Jeff Walden [:Waldo]]

(In reply to comment #11)
> You should be using getBrowser().currentURI.schemeIs("foo") instead

...or perhaps getBrowser.currentURI.scheme and then just compare it in JS.  (/me
stops spamming now)

Comment 13

[Robert Accettura [:raccettura]]

Attachment: Patch v2

Comment 14

[Robert Accettura [:raccettura]]

Attachment: Patch v2.1

Fixes some stupid mistakes (lack of sleep, and a long break from coding isn't
good).

Comment 15

[Robert Accettura [:raccettura]]

Comment on attachment 193409 [details] [diff] [review]
Patch v2.1

Gavin has a better patch

Comment 16

[:Gavin Sharp [email: [e-mail]]]

Sorry, I've been too busy lately, I'll post the patch tonight.

Comment 17

[:Gavin Sharp [email: [e-mail]]]

Attachment: alternate patch

Comment 18

[:Gavin Sharp [email: [e-mail]]]

Attachment: alternate patch v2

Fixes a mistake with the last patch. Not sure who should review this.

Comment 19

[Robert Accettura [:raccettura]]

Gavin: I won't be around much wed/thurs thanks to IBM not fixing my laptop the
first time around... but will be back around thurs evening.

I want this patch in, so if your comfortable that this works with no
regressions, you can r=me if you want (feel free to get another review if you
want a better set of eyes looking at it).  I can't test it, so I'd leave that to
you.  I don't see anything obvious from looking at your patch.  

Ask Asa for approval on the branch.

Comment 20

[Robert Accettura [:raccettura]]

Oh yea... thanks for getting to this

/me grumbles about "depot repair" not being very good... and the desire to have
a dev system again.

Comment 21

[Mike Schroepfer]

Seems like an edge case - is there a more common scenerio here that is tripping
you up?   Also need a review from MConnor when he's back next week.

Comment 22

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 194405 [details] [diff] [review]
alternate patch v2

Fixes this bug, and also the disabled style for the toolbar button.

Comment 23

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 194405 [details] [diff] [review]
alternate patch v2

>Index: extensions/reporter/resources/content/reporter/reporterOverlay.js

>+function onBrowserLoad() {
>+  const nsIWebProgress = Components.interfaces.nsIWebProgress;
>+  gBrowser.addProgressListener(reporterListener, nsIWebProgress.NOTIFY_LOCATION);
>+}

No need for the nsIWebProgress.NOTIFY_LOCATION here, this addProgressListener
only takes a listner and automatically passes _ALL.

Comment 24

[Robert Accettura [:raccettura]]

Gavin, land this when possible.  Get asa's approval for the branch.

Comment 25

[Robert Accettura [:raccettura]]

this really is just polish... can we perhaps get this in?  mconnor? gavin?

Comment 26

[Asa Dotzler [:asa]]

I get a hung reporter window when I try to send in a report for about:blank and
get the error that it's an invalid URL. We need to fix that problem for sure.

Comment 27

[Mike Connor [:mconnor]]

Comment on attachment 194405 [details] [diff] [review]
alternate patch v2

>-function loadReporterWizard()
>-{
>-  window.openDialog("chrome://reporter/content/reportWizard.xul", "", "chrome,centerscreen,dialog,resizable=no,width=535,height=442",getBrowser().currentURI.spec);
>+  QueryInterface: function(aIID) {
>+   if (aIID.equals(Components.interfaces.nsIWebProgressListener) ||
>+       aIID.equals(Components.interfaces.nsISupportsWeakReference) ||
>+       aIID.equals(Components.interfaces.nsISupports))
>+     return this;
>+   throw Components.results.NS_NOINTERFACE;
>+  },

nit: indentation is off by one.

>+  onLocationChange: function(aProgress, aRequest, aURI) {
>+    var broadcaster = document.getElementById("reporterItemsBroadcaster");
>+    // XXX The schemeIs check is only necessary until bug 169826 is fixed,
>+    // since we can end up with a dummy URI here
>+    var isEnabled = (aURI && "schemeIs" in aURI &&
>+                     (aURI.schemeIs("http") ||
>+                      aURI.schemeIs("https") ||
>+                      aURI.schemeIs("ftp") ||
>+                      aURI.schemeIs("gopher")));

is schemeIs expensive here?  is a switch statement faster (this will get called
a lot)

r=me with comments addressed.

Comment 28

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #27)
> is schemeIs expensive here?  is a switch statement faster (this will get called
> a lot)

The four schemeIs()s were faster in my limited testing, though it shouldn't
matter that much since it's being called frequently but not in rapid succession.
New patch coming up.

Comment 29

[:Gavin Sharp [email: [e-mail]]]

Attachment: for checkin

Comment 30

[:Gavin Sharp [email: [e-mail]]]

Trunk:

mozilla/extensions/reporter/resources/content/reporter/reporterOverlay.js
new revision: 1.3; previous revision: 1.2
mozilla/extensions/reporter/resources/content/reporter/reporterOverlay.xul
new revision: 1.6; previous revision: 1.5
mozilla/extensions/reporter/resources/skin/classic/reporter/browserOverlay.css
new revision: 1.2; previous revision: 1.1

Comment 31

[Brendan Eich [:brendan]]

(In reply to comment #28)
> (In reply to comment #27)
> > is schemeIs expensive here?  is a switch statement faster (this will get called
> > a lot)
> 
> The four schemeIs()s were faster in my limited testing,

That's surprising.  Can you attach the alterna-patch that used switch?

/be

Comment 32

[:Gavin Sharp [email: [e-mail]]]

Attachment: use switch instead

Sure. My "limited testing" was sythethic, whether it was representative of
real-world use or not is questionable, I guess, and may have been flawed. If
you prefer this one, here it is.

Comment 33

[:Gavin Sharp [email: [e-mail]]]

Make that |    switch (aURI.scheme || null) {| to avoid the warning.

Comment 34

[Brendan Eich [:brendan]]

Attachment: switch-based benchmark

schemeIs || chain benchmark next.

/be

Comment 35

[Brendan Eich [:brendan]]

Attachment: schemeIs benchmark

Best of 3 in xpcshell, switch time: 441ms, schemeIs time: 767ms -- switch wins,
as expected by me, at least!

/be

Comment 36

[Brendan Eich [:brendan]]

Changing the URI passed to init to be file: (to mismatch all switch cases and
fail all schemeIs tests), best of 3 in xpcshell, switch: 417ms vs. schemeIs:
2886ms(!).

Review-stamping switch patch and nominating it.

/be

Comment 37

[Brendan Eich [:brendan]]

(In reply to comment #33)
> Make that |    switch (aURI.scheme || null) {| to avoid the warning.

Why is this necessary?

Was the schemeIs approach checked into the trunk?  It looks like it.  Reopening
to get the switch approach in.

/be

Comment 38

[:Gavin Sharp [email: [e-mail]]]

Checked in the switch patch, without the || null. Thanks again!

Comment 39

[:Gavin Sharp [email: [e-mail]]]

Attachment: All inclusive branch patch

Comment 40

[Brendan Eich [:brendan]]

(In reply to comment #37)
> (In reply to comment #33)
> > Make that |    switch (aURI.scheme || null) {| to avoid the warning.
> 
> Why is this necessary?

Because of bug 312260.

/be

Comment 41

[:Gavin Sharp [email: [e-mail]]]

Checked in on the branch, with tweak from bug 312353.

mozilla/extensions/reporter/resources/content/reporter/reporterOverlay.js
new revision: 1.2.10.1; previous revision: 1.2
mozilla/extensions/reporter/resources/content/reporter/reporterOverlay.xul
new revision: 1.5.2.1; previous revision: 1.5
mozilla/extensions/reporter/resources/skin/classic/reporter/browserOverlay.css
new revision: 1.1.12.1; previous revision: 1.1

Comment 42

[Robert Accettura [:raccettura]]

gavin: thanks for taking care of this.