Closed
Bug 1322576
Opened 7 years ago
Closed 7 years ago
[geckoview] Add tracking mode boolean in constructor and xml
Categories
(Core Graveyard :: Embedding: APIs, defect)
Core Graveyard
Embedding: APIs
Tracking
(firefox54 fixed)
RESOLVED
FIXED
mozilla54
Tracking | Status | |
---|---|---|
firefox54 | --- | fixed |
People
(Reporter: snorp, Assigned: esawin)
References
Details
Attachments
(5 files, 12 obsolete files)
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.
Comment 1•7 years ago
|
||
(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•7 years ago
|
Assignee: nobody → esawin
Assignee | ||
Comment 2•7 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•7 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•7 years ago
|
||
This adds the GeckoView SafeBrowsing module to handle TP (for now).
Attachment #8837322 -
Flags: review?(snorp)
Assignee | ||
Comment 5•7 years ago
|
||
Fixed rebase.
Attachment #8837319 -
Attachment is obsolete: true
Attachment #8837319 -
Flags: review?(snorp)
Attachment #8837323 -
Flags: review?(snorp)
Assignee | ||
Comment 6•7 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 7•7 years ago
|
||
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+
Reporter | ||
Updated•7 years ago
|
Attachment #8837318 -
Flags: feedback?(snorp) → feedback+
Reporter | ||
Comment 8•7 years ago
|
||
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-
Reporter | ||
Comment 9•7 years ago
|
||
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+
Reporter | ||
Comment 10•7 years ago
|
||
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+
Reporter | ||
Comment 11•7 years ago
|
||
(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.
Reporter | ||
Comment 12•7 years ago
|
||
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•7 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•7 years ago
|
||
Let's make this consistent across the files I'm touching.
Attachment #8837803 -
Flags: review?(bugs)
Comment 15•7 years ago
|
||
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 16•7 years ago
|
||
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•7 years ago
|
||
Addressed comments.
Attachment #8837802 -
Attachment is obsolete: true
Attachment #8838524 -
Flags: review+
Assignee | ||
Comment 18•7 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•7 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•7 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 21•7 years ago
|
||
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 22•7 years ago
|
||
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-
Comment 23•7 years ago
|
||
(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.
Reporter | ||
Comment 24•7 years ago
|
||
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+
Reporter | ||
Comment 25•7 years ago
|
||
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+
Reporter | ||
Comment 26•7 years ago
|
||
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•7 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•7 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•7 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•7 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)
Reporter | ||
Comment 31•7 years ago
|
||
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 32•7 years ago
|
||
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 33•7 years ago
|
||
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•7 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•7 years ago
|
||
Attachment #8839271 -
Attachment is obsolete: true
Attachment #8839664 -
Flags: review?(nchen)
Comment 36•7 years ago
|
||
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 37•7 years ago
|
||
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 38•7 years ago
|
||
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+
Assignee | ||
Comment 39•7 years ago
|
||
Attachment #8839663 -
Attachment is obsolete: true
Attachment #8839923 -
Flags: review+
Assignee | ||
Comment 40•7 years ago
|
||
Attachment #8839664 -
Attachment is obsolete: true
Attachment #8839925 -
Flags: review+
Comment 41•7 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
Comment 42•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/7552fa4a2858 https://hg.mozilla.org/mozilla-central/rev/dd8000832878 https://hg.mozilla.org/mozilla-central/rev/162492fcf4a9 https://hg.mozilla.org/mozilla-central/rev/fb8a931adf8e https://hg.mozilla.org/mozilla-central/rev/6cf3a4f98a62
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Updated•5 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•