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.

Mossop, would this be up your alley for review?

Drew is going to take this. Thanks Drew!

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.

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.

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

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