Closed Bug 1220160 Opened 4 years ago Closed 4 years ago

Figure out how context menu data's docLocation should be set

Categories

(Firefox :: Menus, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 47
Tracking Status
firefox45 --- affected
firefox47 --- fixed

People

(Reporter: Gijs, Assigned: Gijs)

References

Details

Attachments

(3 files)

Maybe the sanest thing would actually be to always use document.documentURI except if that is about:neterror/about:certerror (and then use document.location.href)?

Matt/Boris, does that sound sensible?
Flags: needinfo?(bzbarsky)
Flags: needinfo?(MattN+bmo)
Or better yet we could have a chromeonly getter on document that does the right thing, even in the neterror/certerror case?
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky [:bz] from comment #1)
> Or better yet we could have a chromeonly getter on document that does the
> right thing, even in the neterror/certerror case?

I always appreciate when people point out I'm limiting the scope of my solutions to a problem to a subset of the code we own, to the detriment of the quality of the solution. :-)

I'll have a look at this sometime this week.
Flags: needinfo?(MattN+bmo) → needinfo?(gijskruitbosch+bugs)
Something like this? I'm not sure whether I need to addref for webidl things - I got crashes when I didn't do this for the result of NS_NewURI in an earlier attempt to implement this. I expect it wouldn't crash here either way (though I've not tried) because I'm essentially returning members that would be kept alive by their owning document/channel anyway, but I figured it would make sense to keep the addrefs - maybe it doesn't and I'm missing something because I'm tired. :-\
Attachment #8719615 - Flags: review?(bzbarsky)
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
(In reply to :Gijs Kruitbosch from comment #2)
> I'll have a look at this sometime this week.

Clearly not quite this week. But hey, here's a patch for the core bit. If that looks OK I'll write up the nsContextMenu.js. Oh, and I'll take suggestions for the name of the member variable here... mozDocumentURIBeforeErrors doesn't sound great, but I struggled to come up with something better. :-\
Flags: needinfo?(gijskruitbosch+bugs)
Comment on attachment 8719615 [details] [diff] [review]
part 1: add chromeonly getter for documenturi that returns the original document when on an error page,

>+nsIURI*

Should be already_AddRefed<nsIURI> for the return value.  Otherwise this leaks.

>+  nsCOMPtr<nsIURI> uri = GetDocumentURIObject();

Can't we just move this to after the mFailedChannel block?  I don't see why we care about it until then.

>+      NS_ADDREF(failedURI);
>+      return failedURI;

Please return those two lines with:

  return failedURI.forget();

which will incidentally fail to compile if you don't change the return type to already_AddRefed<nsIURI>, thus catching the leak for you.  ;)

>+  NS_ADDREF(uri);
>+  return uri;

Again, uri.forget().

>+  [ChromeOnly] readonly attribute URI? mozDocumentURIBeforeErrors;

Naming things is hard.  :(

Maybe mozDocumentURIIfNotForErrorPages or something?  Note that for network error pages it doesn't return the "original URI" in the sense of channel originalURI; it returns the URI of the thing we were trying to load when we got the network error, whatever that is.

r=me with those nits.  Thank you for doing this!
Attachment #8719615 - Flags: review?(bzbarsky) → review+
Keywords: leave-open
Attachment #8722137 - Flags: review?(past) → review+
Comment on attachment 8722137 [details]
MozReview Request: Bug 1220160 - part 3: use mozDocumentURIIfNotForErrorPages for ssl error reporting, r?past

https://reviewboard.mozilla.org/r/35879/#review32709

Nice simplification!
Attachment #8722136 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8722136 [details]
MozReview Request: Bug 1220160 - part 2: use mozDocumentURIIfNotForErrorPages for context menu's docLocation, r?MattN

https://reviewboard.mozilla.org/r/35877/#review32791
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/2142620090e7
https://hg.mozilla.org/mozilla-central/rev/e4be65c87fcc
https://hg.mozilla.org/mozilla-central/rev/1b9d91965921
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
You need to log in before you can comment on or make changes to this bug.