DevTools: make disable javascript setting temporary

DevTools: make disable javascript setting temporary

This makes the "disable javascript" setting temporary, similar to
networkBlockedURLs. Before, disabling JS then opening a new tab with DevTools
resulted in disabled JS on the new tab. Now, opening DevTools in the new tab
will not have the same setting as the last tab.

Behavior when closing/opening DevTools on the same tab, navigating the page, and
toggling the checkbox should not be affected by this CL.

BUG=684403
Review-Url: https://codereview.chromium.org/2708413003
Cr-Commit-Position: refs/heads/master@{#454885}
Committed: https://chromium.googlesource.com/chromium/src/+/d86ba3089c88fd85df42b2660bf5dd8a2cda6dfe

[luoe, 2017-02-23 01:06:27]

Description was changed from

==========
DevTools: make disable javascript setting temporary

BUG=684403
==========

to

==========
DevTools: make disable javascript setting temporary

This makes the "disable javascript" setting temporary, similar to
networkBlockedURLs. Before, disabling JS then opening a new tab with DevTools
resulted in disabled JS on the new tab. Now, opening DevTools in the new tab
will not have the same setting as the last tab.

Behavior when closing/opening DevTools on the same tab, navigating the page, and
toggling the checkbox should not be affected by this CL.

BUG=684403
==========

[luoe, 2017-02-23 01:07:06]

luoe@chromium.org changed reviewers:
+ dgozman@chromium.org, pfeldman@chromium.org

[luoe, 2017-02-23 01:07:07]

Please take a look

[pfeldman, 2017-02-23 01:30:51]

https://codereview.chromium.org/2708413003/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/main/Main.js (right):

https://codereview.chromium.org/2708413003/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/main/Main.js:87:
Common.moduleSetting('javaScriptDisabled').set(false);
Should we introduce session settings instead? 'session' = true in descriptor and
.makeSession() in API and not store the actual value in the storage.

[luoe, 2017-02-23 01:33:21]

Nice idea.  There's only 2 places I can see that would use it, here and
networkBlockedURLs.  Please let me know if there are others you know of.

[luoe, 2017-02-25 02:53:03]

The latest patch is an attempt at introducing a temporary SessionStorage.  Since
'session' conflicts with isLocal, a new storageType (local, global, or session)
is passed into createSetting().

This adds a bit of complexity to support just 2 settings javascriptDisabled and
networkBlockedURLs.  I don't think it warrants adding a new storage type,
javascriptDisabled is an exception to the rule.  Most checkboxes in the settings
screen are persistent, while it is the only one that is not.

wdyt/ptal?

https://codereview.chromium.org/2708413003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/common/Settings.js (right):

https://codereview.chromium.org/2708413003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/common/Settings.js:68: }
A very awkward part of this attempt is I've kept checks for the booleans 'local'
and 'session'.

I couldn't find where the module.json format is specified.  Do we have some spec
/ compatibility requirements for our module.jsons?  Are there any other cases
where extensions are defined with enum values?

[pfeldman, 2017-02-27 17:46:33]

This looks good and does not add complexity. In fact, storageForType that you
introduced lowers it.

https://codereview.chromium.org/2708413003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/common/Settings.js (right):

https://codereview.chromium.org/2708413003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/common/Settings.js:41:
this._settingsStorage = globalStorage;
nit: rename this to this._globalStorage while you are here?

https://codereview.chromium.org/2708413003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/common/Settings.js:43:
this._sessionStorage = sessionStorage;
You don't need to pass the session storage from outside, create a blank one here
instead. Prefix is not important here since storage isn't reused.

https://codereview.chromium.org/2708413003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/common/Settings.js:61: var
localType = !!descriptor['local'] ? Common.SettingStorageType.Local :
Common.SettingStorageType.Global;
Use a switch instead for readability.

https://codereview.chromium.org/2708413003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/main/module.json (right):

https://codereview.chromium.org/2708413003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/main/module.json:254: "session":
true,
"type": "session",

https://codereview.chromium.org/2708413003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/sdk/module.json (right):

https://codereview.chromium.org/2708413003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/module.json:13: "session":
true,
ditto

[luoe, 2017-02-27 20:24:34]

ptal

https://codereview.chromium.org/2708413003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/common/Settings.js (right):

https://codereview.chromium.org/2708413003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/common/Settings.js:41:
this._settingsStorage = globalStorage;
On 2017/02/27 17:46:33, pfeldman wrote:
> nit: rename this to this._globalStorage while you are here?

Done.

https://codereview.chromium.org/2708413003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/common/Settings.js:43:
this._sessionStorage = sessionStorage;
On 2017/02/27 17:46:33, pfeldman wrote:
> You don't need to pass the session storage from outside, create a blank one
here
> instead. Prefix is not important here since storage isn't reused.

Done.

https://codereview.chromium.org/2708413003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/common/Settings.js:61: var
localType = !!descriptor['local'] ? Common.SettingStorageType.Local :
Common.SettingStorageType.Global;
On 2017/02/27 17:46:33, pfeldman wrote:
> Use a switch instead for readability.

Done.

https://codereview.chromium.org/2708413003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/main/module.json (right):

https://codereview.chromium.org/2708413003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/main/module.json:254: "session":
true,
"storageType": "session"

https://codereview.chromium.org/2708413003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/sdk/module.json (right):

https://codereview.chromium.org/2708413003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/module.json:13: "session":
true,
On 2017/02/27 17:46:33, pfeldman wrote:
> ditto

Done.

[pfeldman, 2017-03-01 23:23:19]

lgtm % nits

https://codereview.chromium.org/2708413003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/common/Settings.js (right):

https://codereview.chromium.org/2708413003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/common/Settings.js:68: default:
You typically specify all the cases and then return default value for better
readability.

https://codereview.chromium.org/2708413003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/common/Settings.js:158: return
this._globalStorage;
ditto (no fall through). and I would actually default everything to local.

[luoe, 2017-03-03 03:50:23]

https://codereview.chromium.org/2708413003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/common/Settings.js (right):

https://codereview.chromium.org/2708413003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/common/Settings.js:68: default:
On 2017/03/01 23:23:19, pfeldman wrote:
> You typically specify all the cases and then return default value for better
> readability.

Done.

https://codereview.chromium.org/2708413003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/common/Settings.js:158: return
this._globalStorage;
On 2017/03/01 23:23:19, pfeldman wrote:
> ditto (no fall through). and I would actually default everything to local.

Done.

[luoe, 2017-03-03 03:50:29]

The CQ bit was checked by luoe@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-03 03:50:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-03 05:36:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-03 05:36:58]

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_...)

[luoe, 2017-03-03 19:46:57]

Since all settings defaulted to globalStorage before, I don't think this CL
should change the default to localStorage.  The scope here is just adding a new
sessionStorage.

[luoe, 2017-03-03 19:47:03]

The CQ bit was checked by luoe@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-03 19:47:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-03 21:12:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-03 21:12:02]

Dry run: This issue passed the CQ dry run.

[luoe, 2017-03-06 16:28:36]

The CQ bit was checked by luoe@chromium.org

[luoe, 2017-03-06 16:28:37]

The patchset sent to the CQ was uploaded after l-g-t-m from
pfeldman@chromium.org
Link to the patchset: https://codereview.chromium.org/2708413003/#ps140001
(title: "default global")

[commit-bot: I haz the power, 2017-03-06 16:28:57]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-06 18:04:11]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1488817716649540,
"parent_rev": "99ee0570b40f1e61e27a35d8b41c52e892e0fd01", "commit_rev":
"d86ba3089c88fd85df42b2660bf5dd8a2cda6dfe"}

[commit-bot: I haz the power, 2017-03-06 18:04:49]

Description was changed from

==========
DevTools: make disable javascript setting temporary

This makes the "disable javascript" setting temporary, similar to
networkBlockedURLs. Before, disabling JS then opening a new tab with DevTools
resulted in disabled JS on the new tab. Now, opening DevTools in the new tab
will not have the same setting as the last tab.

Behavior when closing/opening DevTools on the same tab, navigating the page, and
toggling the checkbox should not be affected by this CL.

BUG=684403
==========

to

==========
DevTools: make disable javascript setting temporary

This makes the "disable javascript" setting temporary, similar to
networkBlockedURLs. Before, disabling JS then opening a new tab with DevTools
resulted in disabled JS on the new tab. Now, opening DevTools in the new tab
will not have the same setting as the last tab.

Behavior when closing/opening DevTools on the same tab, navigating the page, and
toggling the checkbox should not be affected by this CL.

BUG=684403

Review-Url: https://codereview.chromium.org/2708413003
Cr-Commit-Position: refs/heads/master@{#454885}
Committed:
https://chromium.googlesource.com/chromium/src/+/d86ba3089c88fd85df42b2660bf5...
==========

[commit-bot: I haz the power, 2017-03-06 18:04:50]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/d86ba3089c88fd85df42b2660bf5...