Closed Bug 1170097 Opened 4 years ago Closed 4 years ago

Add originAttributesToCookieJar to ChromeUtils.webidl

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

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
Attached patch Patch. (obsolete) — Splinter Review
Attachment #8613426 - Flags: review?(bobbyholley)
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-
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:
(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
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. :-)
fix some typo.
Attachment #8614613 - Attachment is obsolete: true
Attachment #8614612 - Flags: review?(bobbyholley)
Attachment #8615067 - Flags: review?(bobbyholley)
Attachment #8615068 - Flags: review?(bobbyholley)
Attachment #8614612 - Flags: review?(bobbyholley) → review+
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 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+
Attachment #8614612 - Attachment description: Part 1: Move OriginAttributeDictionary. → Part 2: Move OriginAttributeDictionary.
Attachment #8615067 - Attachment description: Part 2: Add originAttributesToCookieJar → Part 3: Add originAttributesToCookieJar
Attachment #8615068 - Attachment description: Part 3: move cstor/dstor to BasePrincipal.cpp → Part 1: move cstor/dstor to BasePrincipal.cpp
Attachment #8615068 - Attachment is obsolete: true
Attachment #8615786 - Flags: review+
Attachment #8615067 - Attachment is obsolete: true
Attachment #8615788 - Flags: review+
rebase to fix conflict
Attachment #8615788 - Attachment is obsolete: true
Attachment #8615851 - Flags: review+
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)
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
fixed wrong bug number. :P
Attachment #8615786 - Attachment is obsolete: true
Flags: needinfo?(allstars.chh)
Attachment #8617070 - Flags: review+
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 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+
(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.
Summary: Add originAttributesToCookieJar to ChromeUtils.jsm → Add originAttributesToCookieJar to ChromeUtils.webidl
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
Attachment #8617070 - Attachment is obsolete: true
Attachment #8615787 - Attachment description: Part 2: Move OriginAttributeDictionary. → Part 1: Move OriginAttributeDictionary.
Attachment #8617073 - Attachment description: Part 3: Add originAttributesToCookieJar v2 → Part 2: Add originAttributesToCookieJar v2
(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.
(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: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.