skip over master brave window when querying brave/browser-laptop#8811

[kevinlawler]

No description provided.

[jonathansampson]

It may be better to filter on the tab's windowId value. The main Brave window will have a value of -1 while all other tabs should have a positive value. @bridiver, is this correct?

[kevinlawler]

One of the issues with that is -1 already indicates WINDOW_ID_NONE. Normally I would agree, but in this instance it's probably better to filter on something more definitively unique to the main Brave window. (Perhaps it should have a special key set on its object.) That said, -1 will (should) still work if we decide to change it.

[jonathansampson]

@kevinlawler I'm not 100% .url.indexOf('chrome://') will match only the Brave frame. We could also check that the title matches /^brave$/i to be certain.

{
    "active": false,
    "audible": false,
    "autoDiscardable": true,
    "discarded": false,
    "height": 1050,
    "highlighted": false,
    "id": 1,
    "incognito": true,
    "index": -1,
    "mutedInfo": {
        "muted": false,
        "reason": "user"
    },
    "pinned": false,
    "selected": false,
    "status": "complete",
    "title": "Brave",
    "url": "chrome://brave/C:/Users/jjdsa/AppData/Local/brave/app-0.15.303/resources/app.asar/app/extensions/brave/index.html",
    "width": 1922,
    "windowId": -1
}

[kevinlawler]

OK, I'll match against -1 then unless there are objections.

Matching against title=="Brave" I think gives us the same problem before: false positives are improbable but a possible bug nonetheless.

[jonathansampson]

I wasn't suggesting a single check, but rather "chrome://" and "Brave" 🙂 Either one alone could yield false positives indeed. Well, "chrome://" may not—I'm not sure.

Checking the windowId may be safe, but I could be wrong. If it is safe, it's going to be a lot faster than testing two strings. @bridiver or @bbondy may know definitively if any other tab should ever have a windowId value of -1.

[bridiver]

definitely not save to check for indexOf('chrome://'), but indexOf('chrome://brave') would be safe

[jonathansampson]

Thanks, @bridiver. @kevinlawler, how about filtering on tab.url.startsWith('chrome://brave/')?

[bridiver]

sorry I've let this one sit forever. Is this safe to merge for the next build? Doesn't require any browser-laptop changes?

[jonathansampson]

@bridiver No changes to browser-laptop.

[bridiver]

ok, I'll merge and look at removing the TabHelper instead when I have time

[jonathansampson]

Resolves brave/browser-laptop#8822.

@bridiver, do you know when this should appear in preview builds? Still repros for me in 0.19.0 (brave/browser-laptop@c5968c5).

[bridiver]

It should be in 4.1 builds for muon

…

On Jun 18, 2017, at 1:00 PM, Jonathan Sampson ***@***.***> wrote: Resolves brave/browser-laptop#8822. @bridiver, do you know when this should appear in preview builds? Still repros for me in 0.19.0 ***@***.***). — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.