Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2545443002:
Cleanup multiple URLSchemesSet static variables (Closed)
DescriptionCleanup multiple URLSchemesSet static variables
SchemeRegistry.cpp has bunch of distinct static URLSchemesSet's and they
are getting a bit out of control. This patch:
* Introduces URLSchemesRegisry that bundles all the URLSchemesSet's so
that they can be initialized at once
* Provides mutable and non-mutable version of getters for the
URLSchemesRegistry instance so that one can safely call non-mutable
getters without worrying about thread-safety, while we have the
thread assertion in the mutable version of getter.
BUG=n/a
Committed: https://crrev.com/3045382d44f9976637a6922885d27267623d3f06
Cr-Commit-Position: refs/heads/master@{#435618}
Patch Set 1 #
Total comments: 8
Patch Set 2 : addressed comments #Patch Set 3 : re-add fetchAPISchemes initialization #Patch Set 4 : remove a test that expects lower() #
Messages
Total messages: 27 (19 generated)
The CQ bit was checked by kinuko@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Cleanup multiple URLSchemesSet static variables SchemeRegistry.cpp has bunch of distinct static URLSchemesSet's and they are getting a bit out of control. This patch: * Introduces URLSchemesRegisry that bundles all the URLSchemesSet's so that they can be initialized at once * Provides mutable and non-mutable version of getters for the URLSchemesRegistry instance so that one can safely call non-mutable getters without worrying about thread-safety, while we have the thread assertion in the mutable version of getter. BUG=n/a ========== to ========== Cleanup multiple URLSchemesSet static variables SchemeRegistry.cpp has bunch of distinct static URLSchemesSet's and they are getting a bit out of control. This patch: * Introduces URLSchemesRegisry that bundles all the URLSchemesSet's so that they can be initialized at once * Provides mutable and non-mutable version of getters for the URLSchemesRegistry instance so that one can safely call non-mutable getters without worrying about thread-safety, while we have the thread assertion in the mutable version of getter. BUG=n/a ==========
kinuko@chromium.org changed reviewers: + csharrison@chromium.org, mkwst@chromium.org
Can you take a look? Now that the removal of mutex lock is (seemingly) working well I want to clean up the code a little further.
This is a lot cleaner, thank you. LGTM % nit. https://codereview.chromium.org/2545443002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/weborigin/SchemeRegistry.cpp (right): https://codereview.chromium.org/2545443002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/weborigin/SchemeRegistry.cpp:70: friend URLSchemesRegistry& getMutableURLSchemesRegistry(); If the struct has private members and friends, perhaps it should just be a class?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Looks great, thank you! Just a a suggestion and some lower() cleanups I must have missed in a previous patch. https://codereview.chromium.org/2545443002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/weborigin/SchemeRegistry.cpp (right): https://codereview.chromium.org/2545443002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/weborigin/SchemeRegistry.cpp:72: static URLSchemesRegistry& getInstance() { To avoid needing friend functions or private methods, you could rename this method getMutableInstance(), make it public, and provide a second static method getInstance() which calls into this one. https://codereview.chromium.org/2545443002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/weborigin/SchemeRegistry.cpp:338: scheme.lower()); Remove call to lower(). https://codereview.chromium.org/2545443002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/weborigin/SchemeRegistry.cpp:346: scheme.lower()); Remove call to lower(), it is only called with a SecurityOrigin::protocol. Feel free to add a DCHECK here too that the input is lowercase.
The CQ bit was checked by kinuko@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by kinuko@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks! https://codereview.chromium.org/2545443002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/weborigin/SchemeRegistry.cpp (right): https://codereview.chromium.org/2545443002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/weborigin/SchemeRegistry.cpp:70: friend URLSchemesRegistry& getMutableURLSchemesRegistry(); On 2016/11/30 10:27:33, Mike West (slow) wrote: > If the struct has private members and friends, perhaps it should just be a > class? Done. https://codereview.chromium.org/2545443002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/weborigin/SchemeRegistry.cpp:72: static URLSchemesRegistry& getInstance() { On 2016/11/30 13:24:27, Charlie Harrison wrote: > To avoid needing friend functions or private methods, you could rename this > method getMutableInstance(), make it public, and provide a second static method > getInstance() which calls into this one. We want to call isBeforeThreadCreated check in mutable one but not in non-mutable one, so we can't simply/directly make the non-mutable one call the mutable one. https://codereview.chromium.org/2545443002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/weborigin/SchemeRegistry.cpp:338: scheme.lower()); On 2016/11/30 13:24:27, Charlie Harrison wrote: > Remove call to lower(). Done. https://codereview.chromium.org/2545443002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/weborigin/SchemeRegistry.cpp:346: scheme.lower()); On 2016/11/30 13:24:27, Charlie Harrison wrote: > Remove call to lower(), it is only called with a SecurityOrigin::protocol. Feel > free to add a DCHECK here too that the input is lowercase. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
On 2016/12/01 05:53:23, kinuko wrote: > Thanks! > > https://codereview.chromium.org/2545443002/diff/1/third_party/WebKit/Source/p... > File third_party/WebKit/Source/platform/weborigin/SchemeRegistry.cpp (right): > > https://codereview.chromium.org/2545443002/diff/1/third_party/WebKit/Source/p... > third_party/WebKit/Source/platform/weborigin/SchemeRegistry.cpp:70: friend > URLSchemesRegistry& getMutableURLSchemesRegistry(); > On 2016/11/30 10:27:33, Mike West (slow) wrote: > > If the struct has private members and friends, perhaps it should just be a > > class? > > Done. > > https://codereview.chromium.org/2545443002/diff/1/third_party/WebKit/Source/p... > third_party/WebKit/Source/platform/weborigin/SchemeRegistry.cpp:72: static > URLSchemesRegistry& getInstance() { > On 2016/11/30 13:24:27, Charlie Harrison wrote: > > To avoid needing friend functions or private methods, you could rename this > > method getMutableInstance(), make it public, and provide a second static > method > > getInstance() which calls into this one. > > We want to call isBeforeThreadCreated check in mutable one but not in > non-mutable one, so we can't simply/directly make the non-mutable one call the > mutable one. > > https://codereview.chromium.org/2545443002/diff/1/third_party/WebKit/Source/p... > third_party/WebKit/Source/platform/weborigin/SchemeRegistry.cpp:338: > scheme.lower()); > On 2016/11/30 13:24:27, Charlie Harrison wrote: > > Remove call to lower(). > > Done. > > https://codereview.chromium.org/2545443002/diff/1/third_party/WebKit/Source/p... > third_party/WebKit/Source/platform/weborigin/SchemeRegistry.cpp:346: > scheme.lower()); > On 2016/11/30 13:24:27, Charlie Harrison wrote: > > Remove call to lower(), it is only called with a SecurityOrigin::protocol. > Feel > > free to add a DCHECK here too that the input is lowercase. > > Done. LGTM thanks!!
The CQ bit was checked by kinuko@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org, csharrison@chromium.org Link to the patchset: https://codereview.chromium.org/2545443002/#ps60001 (title: "remove a test that expects lower()")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1480602305138300,
"parent_rev": "bff39cd9a3198e07656570ffaf3f62fccb3fcff6", "commit_rev":
"658ae3a889b405400d0312868f7161a2197f6311"}
Message was sent while issue was closed.
Description was changed from ========== Cleanup multiple URLSchemesSet static variables SchemeRegistry.cpp has bunch of distinct static URLSchemesSet's and they are getting a bit out of control. This patch: * Introduces URLSchemesRegisry that bundles all the URLSchemesSet's so that they can be initialized at once * Provides mutable and non-mutable version of getters for the URLSchemesRegistry instance so that one can safely call non-mutable getters without worrying about thread-safety, while we have the thread assertion in the mutable version of getter. BUG=n/a ========== to ========== Cleanup multiple URLSchemesSet static variables SchemeRegistry.cpp has bunch of distinct static URLSchemesSet's and they are getting a bit out of control. This patch: * Introduces URLSchemesRegisry that bundles all the URLSchemesSet's so that they can be initialized at once * Provides mutable and non-mutable version of getters for the URLSchemesRegistry instance so that one can safely call non-mutable getters without worrying about thread-safety, while we have the thread assertion in the mutable version of getter. BUG=n/a ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Cleanup multiple URLSchemesSet static variables SchemeRegistry.cpp has bunch of distinct static URLSchemesSet's and they are getting a bit out of control. This patch: * Introduces URLSchemesRegisry that bundles all the URLSchemesSet's so that they can be initialized at once * Provides mutable and non-mutable version of getters for the URLSchemesRegistry instance so that one can safely call non-mutable getters without worrying about thread-safety, while we have the thread assertion in the mutable version of getter. BUG=n/a ========== to ========== Cleanup multiple URLSchemesSet static variables SchemeRegistry.cpp has bunch of distinct static URLSchemesSet's and they are getting a bit out of control. This patch: * Introduces URLSchemesRegisry that bundles all the URLSchemesSet's so that they can be initialized at once * Provides mutable and non-mutable version of getters for the URLSchemesRegistry instance so that one can safely call non-mutable getters without worrying about thread-safety, while we have the thread assertion in the mutable version of getter. BUG=n/a Committed: https://crrev.com/3045382d44f9976637a6922885d27267623d3f06 Cr-Commit-Position: refs/heads/master@{#435618} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/3045382d44f9976637a6922885d27267623d3f06 Cr-Commit-Position: refs/heads/master@{#435618} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
