Convert PageThumbsProtocol.js to PageThumbsProtocol.cpp

RESOLVED FIXED in Firefox 58

Status

()

defect
P3
normal
RESOLVED FIXED
2 years ago
Last month

People

(Reporter: ursula, Assigned: ursula)

Tracking

(Blocks 2 bugs)

unspecified
Firefox 58
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 unaffected, firefox58 unaffected)

Details

Attachments

(1 attachment)

In preparation for bug 1184701, we should convert the moz-page-thumb:// protocol to C++ so we can make it easier for ourselves to work in e10s. 

This may also address some concerns filed in bug 1355861 about the current performance of the page thumbs protocol. 

Since opening a new tab is a pretty frequent occurrence and (at least in Activity Stream's case) we can be showing upwards of 12 screenshots at a time, the protocol handling should definitely be fast. Especially if we want to make it work in e10s. So porting it over to C++ has a double win there.
Assignee: nobody → usarracini
Mossop, would this be up your alley for review?
Flags: needinfo?(dtownsend)
Attachment #8878506 - Flags: review?(adw)
Drew is going to take this. Thanks Drew!
Flags: needinfo?(dtownsend)
Hi Ursula, I'll try to look at this today or tomorrow, or if you need it more urgently, please let me know.
Thanks! No worries, there's no urgency with landing this patch.
Ursula's working on a regression caused by the patch, so I'll remove myself from review for now.  Feel free to flag me again when you're ready.
Attachment #8878506 - Flags: review?(adw)
Priority: -- → P3
Blocks: 1406134
Attachment #8878506 - Flags: review?(adw)
Should be good for review now Drew. This patch is not time sensitive, so no rush, I know it's a pretty large patch. Thanks!
I'll try to review this today, Ursula.
Comment on attachment 8878506 [details]
Bug 1373258 - Convert PageThumbsProtocol.js to PageThumbsProtocol.cpp

https://reviewboard.mozilla.org/r/149856/#review199188

This all looks OK to me, but I admit my Mozilla C++/XPCOM is rusty.  The only comments I have are style nits, like sometimes you write `nsIURI* uri` and other times it's `nsIURI *uri`, and we usually write `nsCOMPtr<nsIFile>`, not `nsCOMPtr <nsIFile>`, but that's small potatoes.

::: toolkit/components/thumbnails/test/test_thumbnails_interfaces.js
(Diff revision 4)
> -  // first check the protocol handler implements the correct interface
> +  // check the protocol handler implements the correct interface
>    let handler = Services.io.getProtocolHandler("moz-page-thumb");
> -  ok(handler instanceof Ci.nsISubstitutingProtocolHandler,
> -     "moz-page-thumb handler provides substituting interface");
> +  ok(handler instanceof Ci.nsIProtocolHandler,
> +     "moz-page-thumb handler provides a protocol handler interface");
> -
> -  // then check that the file URL resolution works

Why remove these two checks?  It seems important to make sure these URIs still resolve to files (which still happens, right?), and that errors are detected.
Attachment #8878506 - Flags: review?(adw) → review+
Comment on attachment 8878506 [details]
Bug 1373258 - Convert PageThumbsProtocol.js to PageThumbsProtocol.cpp

https://reviewboard.mozilla.org/r/149856/#review199188

> Why remove these two checks?  It seems important to make sure these URIs still resolve to files (which still happens, right?), and that errors are detected.

So originally this protocol was a substituting protocol which resolved to a file protocol, but since we want to be showing thumbnails in the content process (eventually), we won't be able to use that approach anymore, which is why this is no longer a substituting protocol. As a result the first test fails (uri instanceof Ci.nsIURI would make it pass but that's not super helpful). I can try changing the test so that it checks if there's a file for the URL but I think that's covered in other thumbnail tests.

As far as the error cases go, you're right we should keep those in. I'll adapt this test to not use resolveURI but instead call the handler's newChannel function and pass it a couple bad uris to make sure the error cases work as intended.
OK, thanks for the explanation, no need to change the test to check for a file.
Ok updated the patch with a bit of logic in ParseProtocolURL to check that the host is correct before continuing to parse - this was actually existing behaviour in the JS implementation, and since I removed the test I forgot to add it in the new implementation. Both the test and the logic to check are now back in the latest version of the patch. I'm also going to push it to try to make sure everything is still good since I did a rebase on central. Thanks Drew!
Pushed by usarracini@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d05449eb1e03
Convert PageThumbsProtocol.js to PageThumbsProtocol.cpp r=adw
https://hg.mozilla.org/mozilla-central/rev/d05449eb1e03
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/d0dc1eb67db8
Port bug 1373258 to TB/IB/SM [Convert PageThumbsProtocol.js to PageThumbsProtocol.cpp]. rs=bustage-fix
Regressions: 1549386
You need to log in before you can comment on or make changes to this bug.