Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The browser - innerWindowID and outerWindowID (improvements) #941

Conversation

@wolfbeast

This comment has been minimized.

Copy link
Member

wolfbeast commented Mar 7, 2017

Can you please provide an actual description here instead of just links?

!Services.prefs.getBoolPref("browser.tabs.remote")) {
this._outerWindowIDBrowserMap.set(b.outerWindowID, b);
}

This comment has been minimized.

Copy link
@mattatobin

mattatobin Mar 7, 2017

Member

1452-1459: Pale Moon is not e10s compliant so I dunno why this needs to be here.

This comment has been minimized.

@janekptacijarabaci

This comment has been minimized.

Copy link
Contributor Author

janekptacijarabaci commented Mar 7, 2017

Can you please provide an actual description here instead of just links?

I haven't an example about this (currently). But...

Ad https://forums.informaction.com/viewtopic.php?f=10&t=22539#p86754:

NoScript is passing in the innerWindowID property of the gBrowser.selectedBrowser. However, on Pale Moon this property is undefined.

NoScript Security Suite 5.0 (March 3, 2017)
chrome\noscript.jar:content\noscript\MainParent.js (lines 425 - 429):

    try {
      let browser = DOM.mostRecentBrowserWindow.noscriptOverlay.currentBrowser;
      
      payload.innerWindowID = browser.innerWindowID;
    } catch (e) {}
@janekptacijarabaci

This comment has been minimized.

Copy link
Contributor Author

janekptacijarabaci commented Mar 7, 2017

Steps to reproduce:

  1. Open Scratchpad

  2. Menu: "Environment-Browser"

  3. Insert the code:

alert(gBrowser.selectedBrowser.innerWindowID);
  1. Run

  2. The result:

[Pale Moon 27.1.1]
undefined

[Pale Moon 27.2.0a - after this fix]
11
[e.g.]

@wolfbeast

This comment has been minimized.

Copy link
Member

wolfbeast commented Mar 7, 2017

Why is this needed? We aren't an e10s browser, and as such won't have an elaborate inner/outer structure nor a need for it.

This is a good example of what we shouldn't do -- if NoScript wants to be compatible with Pale Moon, they need to treat it for what it is and target it as its own application instead of wanting to wallpaper Firefox-specific methods.

@janekptacijarabaci

This comment has been minimized.

Copy link
Contributor Author

janekptacijarabaci commented Mar 7, 2017

Why is this needed?

You see #941 (comment)
IMHO: It has nothing to do with e10s (only)... It uses many extensions.

@wolfbeast

This comment has been minimized.

Copy link
Member

wolfbeast commented Mar 7, 2017

...
The comment just shows what this code change does, but doesn't explain why we should be doing this.

It has everything to do with e10s because it's part of the structure needed for e10s use... NoScript is obviously targeting e10s in Firefox by doing Window ID accounting needed with remote browsers, but that is N/A for us.

I'll be making a one-time exception here since the change is relatively harmless and the work is already done, but I'm not going to be inclined to accept future PRs that suggest changes to the browser core to cater to assumptions made by specific extensions only targeting Firefox. It is the task of extensions to provide compatible code with the browser it extends, not the other way around!

@wolfbeast wolfbeast merged commit de027d6 into MoonchildProductions:master Mar 7, 2017
@janekptacijarabaci

This comment has been minimized.

Copy link
Contributor Author

janekptacijarabaci commented Mar 7, 2017

@wolfbeast
Thank you (although not for itself).

Ad e10s (and as for me)... If you want, you can also look here (again):
https://forum.palemoon.org/viewtopic.php?f=56&t=12946
(or here: greasemonkey/greasemonkey#2485 - it will be "interesting" times, before version 57)

@wolfbeast

This comment has been minimized.

Copy link
Member

wolfbeast commented Mar 8, 2017

Extension devs will just have to understand that they MUST make separate code paths for Pale Moon and Firefox if they are targeting both. That's the length and breadth of it.

@ThrawnCA

This comment has been minimized.

Copy link

ThrawnCA commented Mar 8, 2017

Extension devs will just have to understand that they MUST

Well, this depends on your point of view. Basically, which devs have more bargaining power; the browser authors, or the extension authors? Either could meet the other in the middle.

I know Pale Moon isn't Firefox, but it has the capability to imitate aspects of Firefox if so inclined. Divergence is sometimes important or even essential, but always painful, so when it's feasible+harmless to mimic Firefox, I think it should continue to be on the table.

@wolfbeast

This comment has been minimized.

Copy link
Member

wolfbeast commented Mar 8, 2017

It doesn't depend on your point of view.

By definition, as the name should already make clear, extensions are continuations of the thing they extend. As such, the extension must be made compatible with what it extends, not the other way around. This has nothing to do with bargaining power.
A browser is perfectly fine without extensions. Contrary, extensions have no purpose without a browser they can plug in to.

Or do you really think that it is a valid viewpoint when you have a plug that doesn't fit into your wall socket, that it is somehow the socket's fault and that it has to be changed to make the plug fit? (and a propos, the socket will happily provide electricity either way whether the plug fits or not)

@mattatobin

This comment has been minimized.

Copy link
Member

mattatobin commented Mar 8, 2017

Wouldn't a simple way to resolve this be to test if it is a thing and if not then do it in a way that Pale Moon accepts?

Won't matter in the long run anyway as you won't have any need to target Firefox's behavior in xul code by the end of the year then you can remove the Firefox specific bits that Pale Moon doesn't have a need to support.

@ThrawnCA

This comment has been minimized.

Copy link

ThrawnCA commented Mar 8, 2017

the extension must be made compatible with what it extends, not the other way around.

Actually the internet is full of compromises for the sake of compatibility. Full from one end to the other, and with a long history of it. Even Google has learned to tread carefully when trying to make everyone update and change. It's all a matter of who is more willing to make the compromises, and how much work is involved.

In this case, the change can either be made in 1 browser, or n extensions, where n is the number of e10s-compatible XUL extensions that choose to jump ship to Pale Moon once Mozilla shafts them. Unless the change is actively harmful, why not put it in the browser? And thus make things easier for said extension authors, who have put in the work to handle e10s and then had the rug pulled out from under them completely. It's not a matter of fault; it's a matter of efficient cooperation.

@wolfbeast

This comment has been minimized.

Copy link
Member

wolfbeast commented Mar 9, 2017

Skewed view there. It's not like this has never worked. n extensions made this change to cater to a change in Firefox that is N/A for us. Targeting Firefox with its change makes us an incompatible target for that particular change. If we start down on this path of making changes to cater to extensions then we will end up being a collection of shims "just so extensions work", which can and will cause issues themselves.

We're also not talking about "the internet" and its compromises (which, by the way, are completely different, and I've adopted contra-spec changes a-plenty to "not break the web") -- this is something that doesn't apply to "the internet", it applies to a very specific and exclusive, browser-only change, see above for an analogy.
Maybe I have break it down to something even simpler of an analogy for you: you're complaining that your iPhone 6 charger cable doesn't fit on your iPhone 5. And instead of just using your iPhone 5 cable, you want to have the iPhone 5 changed so it will accept the iPhone 6 cable. Of course such a change is not by design, may look ugly, and may break being able to use either cable at any time.
Does that help?

(If not, I'm afraid i'm at the end of the amount of time I can sink into trying to explain why this is a bad idea. someone else can continue -- on the forum please)

@ThrawnCA

This comment has been minimized.

Copy link

ThrawnCA commented Mar 10, 2017

Well, that's not a bad analogy, except that a phone accepting various charging cables (at least the major ones, eg mini USB, micro USB, lightning) actually sounds like a good idea.

@mattatobin

This comment has been minimized.

Copy link
Member

mattatobin commented Mar 10, 2017

So.. I have a question. Why did you adapt things for e10s on Firefox? It isn't exactly required for extensions to be multiprocess compatible and everyone who has been paying a lick of attention to even the surface events at Mozilla knew that eventually it wouldn't matter because they were killing all Mozilla technologies.

Why should you consider a platform and target that is rapidly destroying it's own platform a good enough reason to change your code but one that has very high long term potential isn't worth your time to implement little more than an if statement or try/catch.

Going to extraordinary lengths and chasing Firefox compatibility and unprecedented compromise was ok for them.. Why is a little tidbit not alright for us?

The real question is.. Do you actually want to produce a Pale Moon extension? If the answer is yes then please throw your Pale Moon users a bone here. Else, stop pretending you are doing little more than only supporting your own user's freedom and choice ONLY when it is convenient for you.

@ThrawnCA

This comment has been minimized.

Copy link

ThrawnCA commented Mar 12, 2017

Well, I'm not the NoScript dev. Giorgio, who is, actually aims to keep pace and redevelop it as a WebExtension. For now, though, it still can work on Pale Moon and pre-WE Firefox.

@janekptacijarabaci janekptacijarabaci deleted the janekptacijarabaci:browser_innerWindowID_outerWindowID branch Mar 18, 2017
@wolfbeast

This comment has been minimized.

Copy link
Member

wolfbeast commented Apr 4, 2017

While troubleshooting something else with a debug build, I noticed the following:

JavaScript strict warning: chrome://browser/content/tabbrowser.xml, line 1453: ReferenceError: refer
ence to undefined property b.outerWindowID

Could you check and see what the cause is? Seems like something at least is incomplete.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.