Issue 4398 - Fix "view source in a separate window" command

Issue 4398 - Fix "view source in a separate window" command

[kzar, 2016-09-13 09:49:26]

(Not familiar with this code but will try my best to review.)

https://codereview.adblockplus.org/29352667/diff/29352668/lib/aardvark.js
File lib/aardvark.js (right):

https://codereview.adblockplus.org/29352667/diff/29352668/lib/aardvark.js#new...
lib/aardvark.js:709: URI: "view-source:data:text/html;charset=utf-8," +
encodeURIComponent(elem.outerHTML),
How come URI, drawSelection and baseURI are added here? I can't see those in the
old code for earlier versions of Gecko.

[Wladimir Palant, 2016-09-13 10:32:08]

https://codereview.adblockplus.org/29352667/diff/29352668/lib/aardvark.js
File lib/aardvark.js (right):

https://codereview.adblockplus.org/29352667/diff/29352668/lib/aardvark.js#new...
lib/aardvark.js:709: URI: "view-source:data:text/html;charset=utf-8," +
encodeURIComponent(elem.outerHTML),
On 2016/09/13 09:49:26, kzar wrote:
> How come URI, drawSelection and baseURI are added here? I can't see those in
the
> old code for earlier versions of Gecko.

You have to look at the changes introduced in
https://bugzilla.mozilla.org/show_bug.cgi?id=1134585, particularly the new
getSelection function. It's a E10S fix - the View Source dialog is running in
the chrome process, it cannot access the selection (living in the content
process) directly. So rather than accepting a selection it requires the URI to
be displayed - originally that was being done by the dialog code. Note that
there is also some trickery to mark the code to be selected, I skipped that and
set drawSelection to false as it doesn't add any real value.

It's rather unfortunate that we are relying on Firefox implementation details
here. Then again, it's the first time this particular code breaks, the calling
convention for this dialog doesn't change too often.

Thinking a bit more about it, we could probably select this particular node in
the document and trigger the "View Selection Source" command from the context
menu. Not sure whether this would be better however, it would then rely on
application details whereas the current code is only relying on the toolkit code
which is shared among all applications.

[kzar, 2016-09-13 12:06:50]

LGTM

https://codereview.adblockplus.org/29352667/diff/29352668/lib/aardvark.js
File lib/aardvark.js (right):

https://codereview.adblockplus.org/29352667/diff/29352668/lib/aardvark.js#new...
lib/aardvark.js:709: URI: "view-source:data:text/html;charset=utf-8," +
encodeURIComponent(elem.outerHTML),
On 2016/09/13 10:32:08, Wladimir Palant wrote:
> On 2016/09/13 09:49:26, kzar wrote:
> > How come URI, drawSelection and baseURI are added here? I can't see those in
> the
> > old code for earlier versions of Gecko.
> 
> You have to look at the changes introduced in
> https://bugzilla.mozilla.org/show_bug.cgi?id=1134585, particularly the new
> getSelection function. It's a E10S fix - the View Source dialog is running in
> the chrome process, it cannot access the selection (living in the content
> process) directly. So rather than accepting a selection it requires the URI to
> be displayed - originally that was being done by the dialog code. Note that
> there is also some trickery to mark the code to be selected, I skipped that
and
> set drawSelection to false as it doesn't add any real value.
> 
> It's rather unfortunate that we are relying on Firefox implementation details
> here. Then again, it's the first time this particular code breaks, the calling
> convention for this dialog doesn't change too often.
> 
> Thinking a bit more about it, we could probably select this particular node in
> the document and trigger the "View Selection Source" command from the context
> menu. Not sure whether this would be better however, it would then rely on
> application details whereas the current code is only relying on the toolkit
code
> which is shared among all applications.

That makes sense, thanks for explaining. (I tried having a look through the
related Firefox change you linked but obviously couldn't quickly grok it all.
I'll take your word for it!)