Closed
Bug 1170097
Opened 9 years ago
Closed 9 years ago
Add originAttributesToCookieJar to ChromeUtils.webidl
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla41
Tracking | Status | |
---|---|---|
firefox41 | --- | fixed |
People
(Reporter: allstars.chh, Assigned: allstars.chh)
References
Details
Attachments
(2 files, 9 obsolete files)
3.28 KB,
patch
|
allstars.chh
:
review+
|
Details | Diff | Splinter Review |
8.61 KB,
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
See Bobby's comment from https://bugzilla.mozilla.org/show_bug.cgi?id=1168300#c14
Assignee | ||
Comment 1•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Attachment #8613426 -
Flags: review?(bobbyholley)
Comment 2•9 years ago
|
||
Comment on attachment 8613426 [details] [diff] [review] Patch. Review of attachment 8613426 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bindings/Bindings.conf @@ +271,5 @@ > # collection of static methods, so we have this `concrete: False` hack. > 'concrete': False, > 'nativeType': 'mozilla::devtools::ChromeUtils', > + 'implicitJSContext': ['readHeapSnapshot', 'saveHeapSnapshot', > + 'originAttributesToCookieJar'] I don't think you'll need implicitJSContext for this, see further comments. ::: dom/webidl/ChromeUtils.webidl @@ +34,5 @@ > + * opaque identfier. > + * > + * @param originAttrs The originAttributes for the caller. > + */ > + [ChromeOnly] This entire interface is ChromeOnly, so this annotation (and the beginning of the comment above) are redundant. @@ +35,5 @@ > + * > + * @param originAttrs The originAttributes for the caller. > + */ > + [ChromeOnly] > + static DOMString originAttributesToCookieJar(optional OriginAttributesDictionary originAttrs); Make this return a ByteString so that we don't have to bother with the UTF16 conversion. ::: toolkit/devtools/server/ChromeUtils.cpp @@ +433,5 @@ > +/* static */ void > +ChromeUtils::OriginAttributesToCookieJar(GlobalObject& global, > + JSContext* cx, > + const OriginAttributesDictionary& originAttrs, > + nsAString& cookieJar) These should be called |aGlobal|, |aAttrs|, |aCookieJar|. @@ +439,5 @@ > + nsCOMPtr<nsIURI> dummyURI; > + nsresult rv = NS_NewURI(getter_AddRefs(dummyURI), "http://example.com"); > + if (NS_FAILED(rv)) { > + return; > + } This is too much of a hack - we're in C++, and we can make things work exactly how we like. Here's what I propose: (1) Add a copy constructor from OriginAttributesDictionary to OriginAttributes like this: OriginAttributes(const OriginAttributesDictionary& aOther) : OriginAttributesDictionary(aOther) {} (2) Create a method called OriginAttributes::CookieJar(nsACString&). Have BasePrincipal::GetJarPrefix and BasePrincipal::CookieJar forward to that method. (3) Reimplement this method as two lines: OriginAttributes attrs(aAttrs); aAttrs.CookieJar(aCookieJar); ::: toolkit/devtools/server/ChromeUtils.h @@ +15,5 @@ > #include "mozilla/AlreadyAddRefed.h" > #include "mozilla/ErrorResult.h" > #include "mozilla/dom/BindingDeclarations.h" > #include "mozilla/dom/ChromeUtilsBinding.h" > +#include "mozilla/dom/SystemDictionariesBinding.h" In this patch, can you also move OriginAttributes from SystemDictionaries to ChromeUtils, and get rid of SystemDictionaries.webidl? ::: toolkit/devtools/server/tests/unit/test_originAttributesToCookieJar.js @@ +5,5 @@ > + const appId = 12; > + > + var cookieJar_1 = ChromeUtils.originAttributesToCookieJar({appId: appId, > + inBrowser: true}); > + do_check_eq(cookieJar_1, appId + "+t+"); Because the cookieJar representation is going to change, we shouldn't hard-code it here. Instead, we should just make sure that we get the same result with this helper as we do when creating a principal with the same OriginAttributes and accessing .cookieJar.
Attachment #8613426 -
Flags: review?(bobbyholley) → review-
Assignee | ||
Comment 3•9 years ago
|
||
Assignee | ||
Comment 4•9 years ago
|
||
Attachment #8613426 -
Attachment is obsolete: true
Assignee | ||
Comment 5•9 years ago
|
||
Comment on attachment 8614613 [details] [diff] [review] Part 2: Add originAttributesToCookieJar Review of attachment 8614613 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/devtools/server/ChromeUtils.cpp @@ +10,5 @@ > > #include "mozilla/devtools/HeapSnapshot.h" > #include "mozilla/devtools/ZeroCopyNSIOutputStream.h" > #include "mozilla/Attributes.h" > +#include "mozilla/BasePrincipal.h" Right now this line will cause some compilation error will looking into this and attach another patch for fixing this. 0:08.61 In file included from ../../../dist/include/nsISupportsUtils.h:14:0, 0:08.61 from ../../../dist/include/nsCOMPtr.h:30, 0:08.61 from ../../../dist/include/mozilla/dom/BindingDeclarations.h:20, 0:08.61 from /home/allstars/src/mozilla/mozilla-central/toolkit/devtools/server/ChromeUtils.h:17, 0:08.61 from /home/allstars/src/mozilla/mozilla-central/toolkit/devtools/server/ChromeUtils.cpp:6: 0:08.61 ../../../dist/include/nsCOMPtr.h: In instantiation of ‘nsCOMPtr<T>::~nsCOMPtr() [with T = nsIContentSecurityPolicy]’: 0:08.61 ../../../dist/include/mozilla/BasePrincipal.h:64:19: required from here 0:08.62 ../../../dist/include/nsISupportsImpl.h:215:60: error: invalid static_cast from type ‘nsIContentSecurityPolicy*’ to type ‘nsISupports*’ 0:08.62 NS_LogCOMPtrRelease((_c), static_cast<nsISupports*>(_p)) 0:08.62 ^ 0:08.62 ../../../dist/include/nsCOMPtr.h:389:5: note: in expansion of macro ‘NSCAP_LOG_RELEASE’ 0:08.62 NSCAP_LOG_RELEASE(this, mRawPtr); 0:08.62 ^ 0:08.62 In file included from ../../../dist/include/mozilla/dom/BindingDeclarations.h:20:0, 0:08.62 from /home/allstars/src/mozilla/mozilla-central/toolkit/devtools/server/ChromeUtils.h:17, 0:08.62 from /home/allstars/src/mozilla/mozilla-central/toolkit/devtools/server/ChromeUtils.cpp:6: 0:08.62 ../../../dist/include/nsCOMPtr.h:93:39: error: invalid use of incomplete type ‘class nsIContentSecurityPolicy’ 0:08.62 #define NSCAP_RELEASE(this, ptr) (ptr)->Release() 0:08.62 ^ 0:08.62 ../../../dist/include/nsCOMPtr.h:391:7: note: in expansion of macro ‘NSCAP_RELEASE’ 0:08.62 NSCAP_RELEASE(this, mRawPtr); 0:08.62 ^ 0:08.62 In file included from ../../../dist/include/mozilla/BasePrincipal.h:10:0, 0:08.62 from /home/allstars/src/mozilla/mozilla-central/toolkit/devtools/server/ChromeUtils.cpp:14: 0:08.62 ../../../dist/include/nsIPrincipal.h:27:7: error: forward declaration of ‘class nsIContentSecurityPolicy’ 0:08.62 class nsIContentSecurityPolicy; /* forward declaration */ 0:08.62 ^ 0:08.92 0:08.92 In the directory /home/allstars/src/mozilla/mozilla-central/obj-x86_64-unknown-linux-gnu/toolkit/devtools/server 0:08.92 The following command failed to execute properly:
Assignee | ||
Comment 6•9 years ago
|
||
(In reply to Bobby Holley (:bholley) from comment #2) > Comment on attachment 8613426 [details] [diff] [review] > Patch. > Thanks for the great comment, Bobby. I've addressed your comments but still need some time to look into comment 5 once it's fixed I'll send another r?. Thanks
Comment 7•9 years ago
|
||
The BasePrincipal destructor needs to move to BasePrincipal.cpp. Baku also does this in his patch in bug 1155153, but you can add it here too and whoever lands second can resolve the conflicts. :-)
Assignee | ||
Comment 9•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Attachment #8614612 -
Flags: review?(bobbyholley)
Assignee | ||
Updated•9 years ago
|
Attachment #8615067 -
Flags: review?(bobbyholley)
Assignee | ||
Updated•9 years ago
|
Attachment #8615068 -
Flags: review?(bobbyholley)
Updated•9 years ago
|
Attachment #8614612 -
Flags: review?(bobbyholley) → review+
Comment 10•9 years ago
|
||
Comment on attachment 8615067 [details] [diff] [review] Part 3: Add originAttributesToCookieJar Review of attachment 8615067 [details] [diff] [review]: ----------------------------------------------------------------- perfect.
Attachment #8615067 -
Flags: review?(bobbyholley) → review+
Comment 11•9 years ago
|
||
Comment on attachment 8615068 [details] [diff] [review] Part 1: move cstor/dstor to BasePrincipal.cpp Review of attachment 8615068 [details] [diff] [review]: ----------------------------------------------------------------- Nit - this should probably be re-ordered as part 1 so that each commit compiles during bisection.
Attachment #8615068 -
Flags: review?(bobbyholley) → review+
Assignee | ||
Updated•9 years ago
|
Attachment #8614612 -
Attachment description: Part 1: Move OriginAttributeDictionary. → Part 2: Move OriginAttributeDictionary.
Assignee | ||
Updated•9 years ago
|
Attachment #8615067 -
Attachment description: Part 2: Add originAttributesToCookieJar → Part 3: Add originAttributesToCookieJar
Assignee | ||
Updated•9 years ago
|
Attachment #8615068 -
Attachment description: Part 3: move cstor/dstor to BasePrincipal.cpp → Part 1: move cstor/dstor to BasePrincipal.cpp
Assignee | ||
Comment 12•9 years ago
|
||
Attachment #8615068 -
Attachment is obsolete: true
Attachment #8615786 -
Flags: review+
Assignee | ||
Comment 13•9 years ago
|
||
Attachment #8614612 -
Attachment is obsolete: true
Attachment #8615787 -
Flags: review+
Assignee | ||
Comment 14•9 years ago
|
||
Attachment #8615067 -
Attachment is obsolete: true
Attachment #8615788 -
Flags: review+
Assignee | ||
Comment 15•9 years ago
|
||
rebase to fix conflict
Attachment #8615788 -
Attachment is obsolete: true
Attachment #8615851 -
Flags: review+
Comment 16•9 years ago
|
||
https://hg.mozilla.org/integration/b2g-inbound/rev/c9c602344f56 https://hg.mozilla.org/integration/b2g-inbound/rev/78ef803f4f11
Assignee | ||
Comment 17•9 years ago
|
||
(In reply to Pulsebot from comment #16) https://hg.mozilla.org/integration/b2g-inbound/rev/8aaa0a247a6f > https://hg.mozilla.org/integration/b2g-inbound/rev/c9c602344f56 > https://hg.mozilla.org/integration/b2g-inbound/rev/78ef803f4f11
Comment 18•9 years ago
|
||
Backout: https://hg.mozilla.org/integration/b2g-inbound/rev/e9285dba7ba6 https://hg.mozilla.org/integration/b2g-inbound/rev/89af4c72fce5
Comment 19•9 years ago
|
||
Hi, part 1 had a wrong bug number in it :) also had to back this out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=2107407&repo=b2g-inbound
Flags: needinfo?(allstars.chh)
Assignee | ||
Comment 20•9 years ago
|
||
Comment on attachment 8615851 [details] [diff] [review] Part 3: Add originAttributesToCookieJar Review of attachment 8615851 [details] [diff] [review]: ----------------------------------------------------------------- ::: caps/BasePrincipal.h @@ +26,5 @@ > { > mAppId = aAppId; > mInBrowser = aInBrowser; > } > + OriginAttributes(const OriginAttributesDictionary& aOther) need to add 'explicit' to prevent clang-static-analysis error see https://treeherder.mozilla.org/logviewer.html#?job_id=8308189&repo=try
Assignee | ||
Comment 21•9 years ago
|
||
fixed wrong bug number. :P
Attachment #8615786 -
Attachment is obsolete: true
Flags: needinfo?(allstars.chh)
Attachment #8617070 -
Flags: review+
Assignee | ||
Comment 22•9 years ago
|
||
add 'explicit' to fix clang-static-anylysis error per Comment 36. Bobby, sorry for bothering you again can you help to review this patch again? Thanks
Attachment #8615851 -
Attachment is obsolete: true
Attachment #8617073 -
Flags: review?(bobbyholley)
Comment 23•9 years ago
|
||
Comment on attachment 8617073 [details] [diff] [review] Part 2: Add originAttributesToCookieJar v2 Review of attachment 8617073 [details] [diff] [review]: ----------------------------------------------------------------- explicit was the only thing added right? No need to re-request review for trivial changes. :-)
Attachment #8617073 -
Flags: review?(bobbyholley) → review+
Assignee | ||
Comment 24•9 years ago
|
||
(In reply to Bobby Holley (:bholley) from comment #23) > explicit was the only thing added right? Yes > No need to re-request review for > trivial changes. :-) okay, thanks for the quick review.
Updated•9 years ago
|
Summary: Add originAttributesToCookieJar to ChromeUtils.jsm → Add originAttributesToCookieJar to ChromeUtils.webidl
Comment 25•9 years ago
|
||
Hi, in bug 1149294 I am moving the ChromeUtils implementation into dom/base as well as splitting out the thread-safe methods to ThreadSafeChromeUtils, and I think you will probably need to rebase on top of my patches. Sorry! Hope it isn't too inconvenient...
Depends on: 1149294
Assignee | ||
Updated•9 years ago
|
Attachment #8617070 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8615787 -
Attachment description: Part 2: Move OriginAttributeDictionary. → Part 1: Move OriginAttributeDictionary.
Assignee | ||
Updated•9 years ago
|
Attachment #8617073 -
Attachment description: Part 3: Add originAttributesToCookieJar v2 → Part 2: Add originAttributesToCookieJar v2
Comment 26•9 years ago
|
||
https://hg.mozilla.org/integration/b2g-inbound/rev/e0c3fc205ceb https://hg.mozilla.org/integration/b2g-inbound/rev/3119d57ebf19
Assignee | ||
Comment 27•9 years ago
|
||
(In reply to Nick Fitzgerald [:fitzgen][:nf] from comment #25) > Hi, in bug 1149294 I am moving the ChromeUtils implementation into dom/base > as well as splitting out the thread-safe methods to ThreadSafeChromeUtils, > and I think you will probably need to rebase on top of my patches. Sorry! > Hope it isn't too inconvenient... Hi Nick Sorry I plan to push it already but the tree is close recently. In your bug there are still dicussions(ni?) going on, I thought it may take a while so I just pushed my commits. Sorry for the inconvenience.
Comment 28•9 years ago
|
||
(In reply to Yoshi Huang[:allstars.chh] from comment #27) > (In reply to Nick Fitzgerald [:fitzgen][:nf] from comment #25) > > Hi, in bug 1149294 I am moving the ChromeUtils implementation into dom/base > > as well as splitting out the thread-safe methods to ThreadSafeChromeUtils, > > and I think you will probably need to rebase on top of my patches. Sorry! > > Hope it isn't too inconvenient... > > Hi Nick > Sorry I plan to push it already but the tree is close recently. In your bug > there are still dicussions(ni?) going on, I thought it may take a while so I > just pushed my commits. > > Sorry for the inconvenience. That's fine, I thought I was closer to landing than I turned out to be :)
https://hg.mozilla.org/mozilla-central/rev/e0c3fc205ceb https://hg.mozilla.org/mozilla-central/rev/3119d57ebf19
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•