[geckoview] Add tracking mode boolean in constructor and xml

RESOLVED FIXED in Firefox 54

Status

defect
RESOLVED FIXED
3 years ago
3 months ago

People

(Reporter: snorp, Assigned: esawin)

Tracking

unspecified
mozilla54
Dependency tree / graph

Firefox Tracking Flags

(firefox54 fixed)

Details

Attachments

(5 attachments, 12 obsolete attachments)

3.39 KB, patch
smaug
: review+
Details | Diff | Splinter Review
12.16 KB, patch
esawin
: review+
Details | Diff | Splinter Review
2.91 KB, patch
snorp
: review+
Details | Diff | Splinter Review
5.21 KB, patch
esawin
: review+
Details | Diff | Splinter Review
7.85 KB, patch
esawin
: review+
Details | Diff | Splinter Review
This will allow someone to create a GeckoView that has tracking protection enabled.
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #0)
> This will allow someone to create a GeckoView that has tracking protection
> enabled.

All of these attrs.xml-type things are difficult to support while we continue to use moz.build.  There are plans afoot to move Fennec to Gradle (finally!) but I strongly urge any use of Android resources within GeckoView to *not* be part of the GeckoView MVP.
Assignee

Updated

2 years ago
Assignee: nobody → esawin
Assignee

Comment 2

2 years ago
Currently, tracking protection is controlled via two Gecko prefs, which does not allow for each GeckoView to control this setting individually.

With this patch, I'm adding a persistent TP flag to the load context, exposed and controlled through the docshell. This allows each GeckoView to control TP through its top docshell/window.
Attachment #8837318 - Flags: review?(bugs)
Attachment #8837318 - Flags: feedback?(snorp)
Assignee

Comment 3

2 years ago
This adds a GeckoViewSettings class for settings management with tracking protection as its first entry.
Attachment #8837319 - Flags: review?(snorp)
Assignee

Comment 4

2 years ago
This adds the GeckoView SafeBrowsing module to handle TP (for now).
Attachment #8837322 - Flags: review?(snorp)
Assignee

Comment 5

2 years ago
Fixed rebase.
Attachment #8837319 - Attachment is obsolete: true
Attachment #8837319 - Flags: review?(snorp)
Attachment #8837323 - Flags: review?(snorp)
Assignee

Comment 6

2 years ago
Patch 1 lets the prefs (pref-TP) and flag-based (flag-TP) settings co-exist:
1. Enabled pref-TP overrides disabled flag-TP.
2. Enabled flag-TP overrides disabled pref-TP.

Since GeckoView will not use pref-TP and other applications (e.g. Fennec) will not use flag-TP, there shouldn't be any conflict here.
Comment on attachment 8837318 [details] [diff] [review]
0001-Bug-1322576-1.0-Add-tracking-protection-attribute-to.patch

>+LoadContext::SetUseTrackingProtection(bool aUseTrackingProtection)
>+{
>+  MOZ_ASSERT(mIsNotNull);
>+
>+  // We shouldn't need this on parent...
MOZ_ASSERT that this isn't called

>+nsDocShell::GetUseTrackingProtection(bool* aUseTrackingProtection)
> {
>-  if (Preferences::GetBool("privacy.trackingprotection.enabled", false)) {
>-    *aIsTrackingProtectionOn = true;
>-  } else if (UsePrivateBrowsing() &&
>-             Preferences::GetBool("privacy.trackingprotection.pbmode.enabled", false)) {
>-    *aIsTrackingProtectionOn = true;
>-  } else {
>-    *aIsTrackingProtectionOn = false;
>+  RefPtr<nsDocShell> parent = GetParentDocshell();
>+  if (parent) {
>+    return parent->GetUseTrackingProtection(aUseTrackingProtection);
>   }
I don't think this is quite right, especially when we're crossing content-chrome boundary.
Even if this docshell had mUseTrackingProtection set to true, we'd use the value from parent.
Using GetSameTypeParent and not GetParentDocshell would fix the content-chrome boundary issue, and I think
that is enough.


>+nsDocShell::SetUseTrackingProtection(bool aUseTrackingProtection)
>+{
>+  mUseTrackingProtection = aUseTrackingProtection;
Given that this works only for top level (same-type) docshells, assert that this docshell doesn't have same type parent.

>+  bool UseTrackingProtection() {
nit, { goes to its own line

>+    bool usingTP;
please initialize to false

>+OfflineCacheUpdateParent::GetUseTrackingProtection(bool *aUseTrackingProtection)
Nit, 
bool* aUseTrackingProtection
Attachment #8837318 - Flags: review?(bugs) → review+
Attachment #8837318 - Flags: feedback?(snorp) → feedback+
Comment on attachment 8837322 [details] [diff] [review]
0003-Bug-1322576-3.0-Add-GeckoView-safe-browsing-module.-.patch

Review of attachment 8837322 [details] [diff] [review]:
-----------------------------------------------------------------

I think this is ok for when we are toggling on/off after the GV is constructed, but for initial state we need something better that doesn't rely on messages. We should pass it via window args when we create the XUL window. There should be a way to set an initial URI via the xml/ctor too, and we don't want to race that.
Attachment #8837322 - Flags: review?(snorp) → review-
Comment on attachment 8837323 [details] [diff] [review]
0002-Bug-1322576-2.0-Add-GeckoView-settings-management.-r.patch

Review of attachment 8837323 [details] [diff] [review]:
-----------------------------------------------------------------

It seems like we keep thinking of more configuration stuff, so I like this.
Attachment #8837323 - Flags: review?(snorp) → review+
Comment on attachment 8837322 [details] [diff] [review]
0003-Bug-1322576-3.0-Add-GeckoView-safe-browsing-module.-.patch

Review of attachment 8837322 [details] [diff] [review]:
-----------------------------------------------------------------

I guess this patch is ok, actually, we just need an additional one to fix the (currently non-existent) race on startup.
Attachment #8837322 - Flags: review- → review+
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #10)
> Comment on attachment 8837322 [details] [diff] [review]
> 0003-Bug-1322576-3.0-Add-GeckoView-safe-browsing-module.-.patch
> 
> Review of attachment 8837322 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I guess this patch is ok, actually, we just need an additional one to fix
> the (currently non-existent) race on startup.

Thinking about it some more, I think we just want to have one message, GeckoViewSettings:Updated, which has the serialized contents of GeckoViewSettings and is sent any time it changes. For the initial settings, we pass those into the xul window arguments.
I think we should extend nsIAndroidView to have a 'settings' getter, which will just return a JS object that is created from the stuff in GeckoViewSettings that is attached to the current GeckoView.
Assignee

Comment 13

2 years ago
I would like to make sure that's the proper way to check for the same-type-parent.
I'll address the coding style related issues in patch 4.
Attachment #8837318 - Attachment is obsolete: true
Attachment #8837802 - Flags: review?(bugs)
Assignee

Comment 14

2 years ago
Let's make this consistent across the files I'm touching.
Attachment #8837803 - Flags: review?(bugs)
Comment on attachment 8837802 [details] [diff] [review]
0001-Bug-1322576-1.1-Add-tracking-protection-attribute-to.patch

>+nsDocShell::GetUseTrackingProtection(bool* aUseTrackingProtection)
> {
>-  if (Preferences::GetBool("privacy.trackingprotection.enabled", false)) {
>-    *aIsTrackingProtectionOn = true;
>-  } else if (UsePrivateBrowsing() &&
>-             Preferences::GetBool("privacy.trackingprotection.pbmode.enabled", false)) {
>-    *aIsTrackingProtectionOn = true;
>-  } else {
>-    *aIsTrackingProtectionOn = false;
>+  nsCOMPtr<nsIDocShellTreeItem> parentAsItem;
>+  GetSameTypeParent(getter_AddRefs(parentAsItem));
>+  nsCOMPtr<nsIDocShell> parent(do_QueryInterface(parentAsItem));
>+
>+  if (parent && parent != static_cast<nsIDocShell*>(this)) {
>+    return parent->GetUseTrackingProtection(aUseTrackingProtection);
>   }
> 
>+  const bool prefTP = Preferences::GetBool(
>+                        "privacy.trackingprotection.enabled", false);
>+  const bool privPrefTP = UsePrivateBrowsing() &&
>+                          Preferences::GetBool(
>+                            "privacy.trackingprotection.pbmode.enabled", false);
>+
>+  *aUseTrackingProtection = mUseTrackingProtection || prefTP || privPrefTP;
>+  return NS_OK;

I was thinking this some more, and I wonder if we should do


*aUseTrackingProtection  = false;

static bool sPrefsInited = false;
static bool sTPEnabled = false;
static bool sTPInPBEnabled = false;
if (!sPresInited) {
  sPrefsInited = true;
  Preferences::AddBoolVarCache(&sTPEnabled, "privacy.trackingprotection.enabled", false);
  Preferences::AddBoolVarCache(&sTPInPBEnabled, "privacy.trackingprotection.pbmode.enabled", false);
}

if (mUseTrackingProtection || sTPEnabled || (UsePrivateBrowsing() && sTPInPBEnabled)) {
  *aUseTrackingProtection = true;
  return NS_OK;
}

RefPtr<nsDocShell> parent = GetParentDocshell();
if (parent) {
  return parent->GetUseTrackingProtection(aUseTrackingProtection);
}

return NS_OK;


The key difference to the original patch is that parent is checked _after_ checking the prefs / mUseTrackingProtection.
That way SetUseTrackingProtection(true) would affect all its child docshell, also if called in a chrome docshell, and yet
SetUseTrackingProtection(false) in chrome and SetUseTrackingProtection(true) would make content to use tp.
And use of BoolVar cache would keep this still reasonable fast.




>+NS_IMETHODIMP
>+nsDocShell::SetUseTrackingProtection(bool aUseTrackingProtection)
>+{
>+  nsCOMPtr<nsIDocShellTreeItem> parentAsItem;
>+  GetSameTypeParent(getter_AddRefs(parentAsItem));
>+  nsCOMPtr<nsIDocShell> parent(do_QueryInterface(parentAsItem));
>+
>+  MOZ_ASSERT(!parent || parent == static_cast<nsIDocShell*>(this));
If we change GetUseTrackingProtection, we could actually leave this assert out

>+
>+  bool UseTrackingProtection() {
{ goes to its own line


those, r+
Attachment #8837802 - Flags: review?(bugs) → review+
Comment on attachment 8837803 [details] [diff] [review]
0004-Bug-1322576-4.0-Fix-some-coding-style-issues.-r-smau.patch

ah, you fix these here. fine.
Attachment #8837803 - Flags: review?(bugs) → review+
Assignee

Comment 17

2 years ago
Addressed comments.
Attachment #8837802 - Attachment is obsolete: true
Attachment #8838524 - Flags: review+
Assignee

Comment 18

2 years ago
I rewrote the settings management to be more easily extendable and better support initial settings during window creation.

GeckoViewSettings now dispatches all settings when one settings has changed.
GeckoViewModule handles the settings update events and provides a callback for modules to react to changes.
Attachment #8837323 - Attachment is obsolete: true
Attachment #8838526 - Flags: review?(snorp)
Assignee

Comment 19

2 years ago
Updated the SafeBrowsing module based on new settings update callback.
Attachment #8837322 - Attachment is obsolete: true
Attachment #8838528 - Flags: review?(snorp)
Assignee

Comment 20

2 years ago
Pass the initial settings to nsWindow::GeckoViewSupport::Open.

I took the easy way here and passed it as a JSON string as I don't see any benefit in doing anything more complex since we'll pass the whole settings on each update through the event dispatcher.

The missing part now is to allow to provide the initial settings before window construction.
Attachment #8838529 - Flags: review?(nchen)
Attachment #8838529 - Flags: feedback?(snorp)
Comment on attachment 8838529 [details] [diff] [review]
0005-Bug-1322576-5.0-Pass-initial-GeckoView-settings-when.patch

Review of attachment 8838529 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoView.java
@@ +64,4 @@
>          /* package */ Window() {}
>  
>          static native void open(Window instance, GeckoView view, Object compositor,
> +                                EventDispatcher dispatcher, String chromeURI, String settings, int screenId);

Preferably you should pass in a GeckoBundle, and use the EventDispatcher code to turn the bundle into a jsval/jsobject.

::: widget/android/nsIAndroidBridge.idl
@@ +65,4 @@
>  [scriptable, uuid(60a78a94-6117-432f-9d49-304913a931c5)]
>  interface nsIAndroidView : nsIAndroidEventDispatcher
>  {
> +  readonly attribute string settings;

Should be `jsval` so JS code can access it directly.
Attachment #8838529 - Flags: review?(nchen) → feedback+
Comment on attachment 8838526 [details] [diff] [review]
0002-Bug-1322576-2.1-Add-GeckoView-settings-management.-r.patch

Review of attachment 8838526 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoView.java
@@ +10,4 @@
>  
>  import org.json.JSONException;
>  import org.json.JSONObject;
> +import org.mozilla.gecko.GeckoViewSettings;

Don't need this

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoViewSettings.java
@@ +10,5 @@
> +
> +import org.json.JSONObject;
> +import org.json.JSONException;
> +
> +import android.support.v4.util.SimpleArrayMap;

Don't need this

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/util/GeckoBundle.java
@@ +361,5 @@
> +     * @param value Value to map to.
> +     *
> +     * @return Old value for the given key.
> +     */
> +    public Object put(final String key, final Object value) {

GeckoBundle doesn't have this method by design, because it doesn't support all Object values. We also don't want people to, e.g., set USE_TRACKING_PROTECTION to a String or a random Object. So you should check for proper data types, and use the corresponding put* method in GeckoBundle.
Attachment #8838526 - Flags: feedback-
(In reply to Jim Chen [:jchen] [:darchons] from comment #21)
> Comment on attachment 8838529 [details] [diff] [review]
> 0005-Bug-1322576-5.0-Pass-initial-GeckoView-settings-when.patch
> 
> >          static native void open(Window instance, GeckoView view, Object compositor,
> > +                                EventDispatcher dispatcher, String chromeURI, String settings, int screenId);
> 
> Preferably you should pass in a GeckoBundle, and use the EventDispatcher
> code to turn the bundle into a jsval/jsobject.
> 
> > +  readonly attribute string settings;
> 
> Should be `jsval` so JS code can access it directly.

Ah I think I misread. Since you're just passing the JSON in as the initial setting, I would just use the second argument (window.argument[1]) instead of changing nsIAndroidView. To me, nsIAndroidView.setting implies it holds the current setting but it does not.
Comment on attachment 8838526 [details] [diff] [review]
0002-Bug-1322576-2.1-Add-GeckoView-settings-management.-r.patch

Review of attachment 8838526 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good modulo jchen's comments

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoViewSettings.java
@@ +37,5 @@
> +        mBundle = new GeckoBundle();
> +        set(Key.USE_TRACKING_PROTECTION, false);
> +    }
> +
> +    public void set(Key key, Object value) {

As jchen says, use a type-specific function here (setBoolean in this case)

::: mobile/android/modules/GeckoViewModule.jsm
@@ +39,5 @@
> +  updateSettings(settings) {
> +    if (!settings) {
> +      // Initialize settings through the AndroidView interface. The settings
> +      // are passed through as a JSON string when opening the associated window.
> +      let view = this.window.arguments[0].QueryInterface(Ci.nsIAndroidView);

Not part of this patch, but maybe not a big deal
Attachment #8838526 - Flags: review?(snorp) → review+
Comment on attachment 8838529 [details] [diff] [review]
0005-Bug-1322576-5.0-Pass-initial-GeckoView-settings-when.patch

Review of attachment 8838529 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good with jchen's suggestions

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoView.java
@@ +64,4 @@
>          /* package */ Window() {}
>  
>          static native void open(Window instance, GeckoView view, Object compositor,
> +                                EventDispatcher dispatcher, String chromeURI, String settings, int screenId);

Yeah, I like this idea.
Attachment #8838529 - Flags: feedback?(snorp) → feedback+
Comment on attachment 8838528 [details] [diff] [review]
0003-Bug-1322576-3.1-Add-GeckoView-safe-browsing-module.-.patch

Review of attachment 8838528 [details] [diff] [review]:
-----------------------------------------------------------------

I think maybe we want a settings content script that gets the settings proxied from chrome, then applies them as necessary. Not individual modules for every setting. This way we can also ensure TP is set up appropriately before navigating to the initial URI, for instance, since it's all in one place.

::: mobile/android/modules/GeckoViewSafeBrowsing.jsm
@@ +31,5 @@
> +
> +  onSettingsUpdate() {
> +    debug("onSettingsUpdate: " + JSON.stringify(this.settings));
> +
> +    this.useTrackingProtection = this.settings.useTrackingProtection;

Below you coerce to bool in the setter, but better to do it here with !!this.settings.useTrackingProtection IMO

@@ +44,5 @@
> +    if (use) {
> +      this.ensureInit();
> +    }
> +    if (use != this._useTrackingProtection) {
> +      this.messageManager.loadFrameScript('data:,' +

What are the implications of loading a bunch of data: scripts like this? It feels better to have a script there listening for the settings change itself
Attachment #8838528 - Flags: review?(snorp) → review-
Assignee

Comment 27

2 years ago
Addressed comments.

The settings are now exposed as a read-only object and we no longer pass the settings bundle through the event dispatcher (patch 2.2).
Attachment #8838529 - Attachment is obsolete: true
Attachment #8839271 - Flags: review?(nchen)
Assignee

Comment 28

2 years ago
Addressed comments.

Getter/Setter are now type-based and we no longer send the bundle through the event dispatcher.
Attachment #8838526 - Attachment is obsolete: true
Attachment #8839272 - Flags: review?(nchen)
Assignee

Comment 29

2 years ago
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #26)
> I think maybe we want a settings content script that gets the settings
> proxied from chrome, then applies them as necessary. Not individual modules
> for every setting.

Having a general settings module would inherently mean a mixed-context module that handles mechanics and keeps state that's independent from each other. I think having each context module handle its specific settings would result in stronger and cleaner separation between the modules.

> This way we can also ensure TP is set up appropriately
> before navigating to the initial URI, for instance, since it's all in one
> place.

What would be different with a general settings module, the order of initialization? That seems like a solvable issue and probably something that will need to be addressed independent from settings handling at some point.

> Below you coerce to bool in the setter, but better to do it here with
> !!this.settings.useTrackingProtection IMO

Fixed in 3.2.

> What are the implications of loading a bunch of data: scripts like this? It
> feels better to have a script there listening for the settings change itself.

I don't know about the implications but I would assume the side effects to be minimal, since this is only called when the settings are changed by the app.
We can add a content script to handle it, but it feels like unnecessary bloat as long as that's the only frame script we run in this module.
Assignee

Comment 30

2 years ago
Use a general settings module to handle GeckoView settings.
By keeping the onSettingsUpdate callback in GeckoViewModule, we allow for other modules to handle their settings individually, if preferred.
Attachment #8838528 - Attachment is obsolete: true
Attachment #8839472 - Flags: review?(snorp)
Comment on attachment 8839472 [details] [diff] [review]
0003-Bug-1322576-3.3-Add-GeckoView-settings-module.-r-sno.patch

Review of attachment 8839472 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah, I like this better.
Attachment #8839472 - Flags: review?(snorp) → review+
Comment on attachment 8839272 [details] [diff] [review]
0002-Bug-1322576-2.2-Add-GeckoView-settings-management.-r.patch

Review of attachment 8839272 [details] [diff] [review]:
-----------------------------------------------------------------

This looks fine, but I don't think it's thread-safe right now because you're accessing the same bundle from UI and Gecko threads. You'd need to add some kind of locking (or go back to sending a copy).

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoViewSettings.java
@@ +8,5 @@
> +
> +import org.mozilla.gecko.util.GeckoBundle;
> +
> +import org.json.JSONObject;
> +import org.json.JSONException;

Not used

@@ +28,5 @@
> +    public static final Key<Boolean> USE_TRACKING_PROTECTION =
> +        new Key<Boolean>("useTrackingProtection");
> +
> +    private EventDispatcher mEventDispatcher;
> +    private GeckoBundle mBundle;

final

@@ +50,5 @@
> +    public Object getBoolean(Key<Boolean> key) {
> +        return mBundle.getBoolean(key.text);
> +    }
> +
> +    public GeckoBundle asBundle() {

Probably want package-private for now

@@ +55,5 @@
> +        return mBundle;
> +    }
> +
> +    private void dispatchUpdate() {
> +        if (mEventDispatcher == null) {

I don't think this will ever be null

@@ +58,5 @@
> +    private void dispatchUpdate() {
> +        if (mEventDispatcher == null) {
> +            return;
> +        }
> +        mEventDispatcher.dispatch("GeckoView:SettingsUpdate", null);

"GeckoView:UpdateSettings"

::: mobile/android/modules/GeckoViewModule.jsm
@@ +20,5 @@
>      this.window = window;
>      this.browser = browser;
>      this.eventDispatcher = eventDispatcher;
>  
> +    this.eventDispatcher.registerListener(this, [

You can also do,

    this.eventDispatcher.registerListener(() => this.onSettingsUpdate(), "GeckoView:SettingsUpdate");
Attachment #8839272 - Flags: review?(nchen) → feedback+
Comment on attachment 8839271 [details] [diff] [review]
0005-Bug-1322576-5.1-Make-GeckoView-settings-accessible-t.patch

Review of attachment 8839271 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/android/EventDispatcher.cpp
@@ +985,5 @@
> +nsresult
> +EventDispatcher::UnboxBundle(JSContext* aCx, jni::Object::Param aBundle,
> +                             JS::MutableHandleValue aOut)
> +{
> +    return UnboxData(aCx, aBundle, aOut, true);

Can't you just call `UnboxBundle` directly and not have to make any changes to `UnboxData`?

::: widget/android/nsIAndroidBridge.idl
@@ +66,3 @@
>  interface nsIAndroidView : nsIAndroidEventDispatcher
>  {
> +  readonly attribute jsval settings;

Use `[implicit_jscontext]` to get a JSContext* for your getter, instead of using `AutoJSAPI`
Attachment #8839271 - Flags: review?(nchen) → feedback+
Assignee

Comment 34

2 years ago
Thanks for the review, I've addressed it all, I think.
Attachment #8839272 - Attachment is obsolete: true
Attachment #8839663 - Flags: review?(nchen)
Assignee

Comment 35

2 years ago
Attachment #8839271 - Attachment is obsolete: true
Attachment #8839664 - Flags: review?(nchen)
Comment on attachment 8839663 [details] [diff] [review]
0002-Bug-1322576-2.3-Add-GeckoView-settings-management.-r.patch

Review of attachment 8839663 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoViewSettings.java
@@ +24,5 @@
> +
> +    public static final Key<Boolean> USE_TRACKING_PROTECTION =
> +        new Key<Boolean>("useTrackingProtection");
> +
> +    private EventDispatcher mEventDispatcher;

final too
Attachment #8839663 - Flags: review?(nchen) → review+
Comment on attachment 8839663 [details] [diff] [review]
0002-Bug-1322576-2.3-Add-GeckoView-settings-management.-r.patch

Review of attachment 8839663 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/modules/GeckoViewModule.jsm
@@ +36,5 @@
> +  onSettingsUpdate() {}
> +
> +  get settings() {
> +    let view = this.window.arguments[0].QueryInterface(Ci.nsIAndroidView);
> +    return view.settings;

BTW, you probably want to use `Object.freeze` to freeze the returned JS object.
Comment on attachment 8839664 [details] [diff] [review]
0005-Bug-1322576-5.2-Make-GeckoView-settings-accessible-t.patch

Review of attachment 8839664 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM

::: widget/android/nsIAndroidBridge.idl
@@ +62,4 @@
>                            in jsval events);
>  };
>  
> +[scriptable, uuid(abe3762b-f5e1-4020-8970-83b09ac49bbb)]

Don't need to bump the uuid

@@ +66,4 @@
>  interface nsIAndroidView : nsIAndroidEventDispatcher
>  {
> +  [implicit_jscontext]
> +  readonly attribute jsval settings;

Nit: one line

::: widget/android/nsWindow.cpp
@@ +954,5 @@
> +
> +    JNIEnv* const env = jni::GetGeckoThreadEnv();
> +    env->MonitorEnter(mSettings.Get());
> +    nsresult rv = widget::EventDispatcher::UnboxBundle(aCx, mSettings, aOut);
> +    env->MonitorExit(mSettings.Get());

Can you file a follow-up to add wrappers for these in jni/Refs.h?
Attachment #8839664 - Flags: review?(nchen) → review+

Comment 41

2 years ago
Pushed by esawin@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7552fa4a2858
[1.2] Add tracking protection attribute to nsILoadContext to allow for overriding of the global preference setting for individual DocShells. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/dd8000832878
[2.4] Add GeckoView settings management. r=snorp,jchen
https://hg.mozilla.org/integration/mozilla-inbound/rev/162492fcf4a9
[3.3] Add GeckoView settings module. r=snorp
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb8a931adf8e
[4.0] Fix some coding style issues. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/6cf3a4f98a62
[5.3] Make GeckoView settings accessible through nsIAndroidView. r=jchen

Updated

3 months ago
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.