It should be possible to disable browser cache directly from the Network panel. The current checkbox in the Options panel can stay and needs to be synchronized with the new UI in the Net panel. Honza

Hi Honza, should this be added to LaunchPad?

(In reply to Marco Mucci [:MarcoM] from comment #1) > Hi Honza, should this be added to LaunchPad? yes Thanks! Honza

Since bug 1350228 is done, we can bring `disabling browser cache` checkbox in netmonitor

Hi.. Can I take up this bug? I am a beginner (have worked on 6-7 moz bugs only).

Of course! Thanks for contribution Honza, do you think we have enough space to show this option on the top right? or should we save the space for throttle control(from rwd view) first?

(In reply to Fred Lin [:gasolin] from comment #5) > Of course! Thanks for contribution > > Honza, do you think we have enough space to show this option on the top > right? or should we save the space for throttle control(from rwd view) first? Yes, I think there is enough space in the toolbar and we can go ahead and put the checkbox "Disable cache" in there. Honza

Following from comment#6, after doing a bit of digging, I guess the following has to be done: 1. Add checkbox UI to http://searchfox.org/mozilla-central/source/devtools/client/locales/en-US/netmonitor.properties 2. Set checkbox behaviour and link new UI checkbox with old checkbox using http://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/components/toolbar.js I might be missing some things, but if the above steps seem to be in the right direction, then I'll go ahead making the changes and uploading the patch. :)

Yes, beside the above change, you can also * use `Services.prefs.addObserver` to monitor the preference in toolbar.js, and update UI accordingly (can refer devtools/client/aboutdebugging/components/addons/panel.js) * dispatch an redux action while user check/uncheck the checkbox * set the pref in `src/middleware/prefs.js` while receive the correct action

Comment on attachment 8870533 [details] Bug 1349561 - Add UI for disabling browser cache in Net panel in MozReview; I've made quite some changes in this patch. I have got the checkbox UI to show up in the toolbar, but I am not very sure if clicking it performs the disabling. Also, I am not sure about the setting of the preference as I couldn't find one. Do I have to add a new preference?

Comment on attachment 8870533 [details] Bug 1349561 - Add UI for disabling browser cache in Net panel in MozReview; https://reviewboard.mozilla.org/r/141984/#review145870 Thanks for provide the patch! I saw you need some hint to find the actual pref: 1. open network monitor in browser > open the `toolbox options` at top right. 2. check the advanced settings section, find the pref behind the `Disable HTTP Cache` checkbox You can use http://searchfox.org/ to trace where the pref behind the checkbox, or use the browser toolbox http://searchfox.org/ to find it Once you find it, update the middleware/pref.js, and use `Services.prefs.addObserver` to monitor the preference in toolbar.js (can refer devtools/client/aboutdebugging/components/addons/panel.js) Feel free to needinfo me or discuss on irc #devtools or slack #netmonitor https://devtools-html.slack.com/ ::: devtools/client/locales/en-US/netmonitor.properties:604 (Diff revision 1) > # shortcut key to focus on the toolbar url filtering textbox > netmonitor.toolbar.filterFreetext.key=CmdOrCtrl+F > > +# LOCALIZATION NOTE (netmonitor.toolbar.checkBox.label): This is the label > +# displayed for the checkbox for disabling browser cache. > +netmonitor.toolbar.checkBox.label=Disable cache we will have more checkbox after this patch, please use a more clear string name to present the checkbox text and tooltip ::: devtools/client/netmonitor/src/actions/ui.js:34 (Diff revision 1) > } > > /** > + * Change browser cache state. > + * > + * @param {boolean} disable - expected browser cache in disable state its not clear what `disable = true` means, please use `disabled` instead ::: devtools/client/netmonitor/src/assets/styles/netmonitor.css:765 (Diff revision 1) > +.devtools-checkbox { > + vertical-align: middle; > +} > + > +.devtools-checkbox-label { > + margin: 1px 3px 1px 1px For rtl friendly, please use margin-inline-start / margin-inline-end to replace margin-left/margin-right ::: devtools/client/netmonitor/src/components/toolbar.js:147 (Diff revision 1) > > module.exports = connect( > (state) => ({ > networkDetailsToggleDisabled: isNetworkDetailsToggleButtonDisabled(state), > networkDetailsOpen: state.ui.networkDetailsOpen, > + browserCacheDisable: state.ui.browserCacheDisable, use `browserCacheDisabled` ::: devtools/client/netmonitor/src/middleware/prefs.js:13 (Diff revision 1) > const { > ENABLE_REQUEST_FILTER_TYPE_ONLY, > RESET_COLUMNS, > TOGGLE_COLUMN, > TOGGLE_REQUEST_FILTER_TYPE, > + DISABLE_BROWSER_CACHE, `TOGGLE_BROWSER_CACHE` is more accurate to what this action does ::: devtools/client/netmonitor/src/middleware/prefs.js:36 (Diff revision 1) > Services.prefs.setCharPref( > "devtools.netmonitor.filters", JSON.stringify(filters)); > break; > + case DISABLE_BROWSER_CACHE: > + Services.prefs.setCharPref( > + "devtools.netmonitor.disableBrowserCache", JSON.stringify(filters)); That is not right. There should be something like ``` Services.prefs.setBoolPref("?", store.getState().ui.browserCacheDisabled); ```

browser toolbox doc on MDN https://developer.mozilla.org/en-US/docs/Tools/Browser_Toolbox

Comment on attachment 8870533 [details] Bug 1349561 - Add UI for disabling browser cache in Net panel in MozReview; Thanks for the guidance! Actually, the pref was right in front of me but somehow I missed it. I've incorporated changes as per comment#11 in this patch. Do let me know if I need to make any additional changes or if I missed something.

Comment on attachment 8870533 [details] Bug 1349561 - Add UI for disabling browser cache in Net panel in MozReview; Sorry, there was one typo in devtools/client/netmonitor/src/reducers/ui.js in Diff 2 and a string correction in devtools/client/netmonitor/src/components/toolbar.js which is fixed in this patch. One thing that I observed was that when I checked the checkbox in the toolbar, the corresponding checkbox in Settings didn't get checked on its own (and this differs the expected behaviour as both should be linked). I tried adding some changes to http://searchfox.org/mozilla-central/source/devtools/client/framework/toolbox-options.js#117 but no success. Any suggestions?

Comment on attachment 8870533 [details] Bug 1349561 - Add UI for disabling browser cache in Net panel in MozReview; https://reviewboard.mozilla.org/r/141984/#review149116 Good to see some progress there! Can you help check if we did set the right pref value in middleware if the corresponding checkbox in Settings didn't get checked? ::: devtools/client/netmonitor/src/components/toolbar.js:84 (Diff revision 3) > ]; > if (!networkDetailsOpen) { > toggleButtonClassName.push("pane-collapsed"); > } > > + Services.prefs.addObserver(DEVTOOLS_DISABLE_CACHE_PREF, { instead of add a object, please refer follow link to write a callback function http://searchfox.org/mozilla-central/source/devtools/client/framework/toolbox-options.js `render` is not the right place to do the observer, please do so in react `componentDidMount` method and add removeObserver at `componentWillUnmount` method. ::: devtools/client/netmonitor/src/components/toolbar.js:87 (Diff revision 3) > } > > + Services.prefs.addObserver(DEVTOOLS_DISABLE_CACHE_PREF, { > + observe: (...args) => { > + let cacheDisabled = !Services.prefs.getBoolPref(DEVTOOLS_DISABLE_CACHE_PREF); > + this.setState({ cacheDisabled }); do you mean {browserCacheDisabled}? instead of setState, we should dispatch the state change to redux via disableBrowserCache, with that way we can maintain a single store ::: devtools/client/netmonitor/src/components/toolbar.js:134 (Diff revision 3) > + { > + className: "devtools-checkbox-label", > + title: L10N.getStr("netmonitor.toolbar.disableCache.tooltip"), > + }, > + input({ > + className: "devtools-checkbox", should add `checked` attr based on {browserCacheDisabled}

Comment on attachment 8870533 [details] Bug 1349561 - Add UI for disabling browser cache in Net panel in MozReview; Please ignore diff#4 & diff#5 as I did a hg pull on my local repo and that altered the commits. Patch diff to be reviewed is https://reviewboard.mozilla.org/r/141984/diff/3-6/ I have checked and the pref that we are trying to set seems correct (http://searchfox.org/mozilla-central/source/devtools/client/framework/toolbox-options.js#115). I guess we have to set the checkbox (similar to http://searchfox.org/mozilla-central/source/devtools/client/framework/toolbox-options.js#117). I have tried that but it didn't seem to take any effect.

Comment on attachment 8870533 [details] Bug 1349561 - Add UI for disabling browser cache in Net panel in MozReview; https://reviewboard.mozilla.org/r/141984/#review149556 please go to mozreview and check issues are all addressed ::: devtools/client/netmonitor/src/components/toolbar.js:130 (Diff revision 4) > }, > input({ > className: "devtools-checkbox", > + id: "devtools-cache-checkbox", > type: "checkbox", > + checked: browserCacheDisabled, You can use `defaultChecked` attribute to show uncheck/checked state for react input component. ::: devtools/client/netmonitor/src/components/toolbar.js:158 (Diff revision 4) > + }, > + > + updateBrowserCacheDisabled() { > + let browserCacheDisabled = Services.prefs.getBoolPref("devtools.cache.disabled"); > + let browserCacheDisabledCheckbox = this.doc.getElementById("devtools-cache-checkbox"); > + browserCacheDisabledCheckbox.checked = browserCacheDisabled; this line is not necessary since we can set check state via input attribute ::: devtools/client/netmonitor/src/assets/styles/netmonitor.css:765 (Diff revision 5) > +.devtools-checkbox { > + vertical-align: middle; > +} > + > +.devtools-checkbox-label { > + margin: 1px 3px 1px 1px please use rtl friendly syntax such as margin-inline-start (as previous PR version)

(In reply to Fred Lin [:gasolin] from comment #21) > > ::: devtools/client/netmonitor/src/assets/styles/netmonitor.css:765 > (Diff revision 5) > > +.devtools-checkbox { > > + vertical-align: middle; > > +} > > + > > +.devtools-checkbox-label { > > + margin: 1px 3px 1px 1px > > please use rtl friendly syntax such as margin-inline-start (as previous PR > version) Actually, the rtl friendly version is in (Diff revision 6) (Diff revision 4 & 5 were published by mistake). Also, sorry for the delay in notifying, but some important college related work has popped up, so I won't be able to give much time to moz bugs till next week. I am okay if someone else wants to work on this bug during that period.

Thank you for already resolve most issues. There's no schedule for this feature, so feel free to pick it up when you have time.

Comment on attachment 8880095 [details] Bug 1349561 - Add UI for disabling browser cache in Net panel; My local repository got messed up and the earlier mozreview request had also become shabby. So I have created this fresh review request. The changes in this patch are producing the expected behavior. Let me know if the changes look fine, I'll give a run on Try.

Sorry for late review, The patch looks good, please rebased to latest branch and we could run some test to make sure everything works fine

Comment on attachment 8880095 [details] Bug 1349561 - Add UI for disabling browser cache in Net panel; remove the review flag, please rebase first then set the flag again

(In reply to Fred Lin [:gasolin] from comment #26) > Sorry for late review, The patch looks good, please rebased to latest branch > and we could run some test to make sure everything works fine Actually my laptop hard disk failed last week and I lost all my data. So, I'll be creating a fresh review request. Sorry for the problem caused. After integrating the patch to the latest branch, eslint is giving me the following error: > 157:5 error 'store' is not defined. no-undef (eslint) I tried doing something similar to https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/index.js#49 but the store does not get updated when I change the pref from 'Toolbox Options'. Any suggestions?

Sorry, I forgot to mention in comment#28 that the error is coming from the file devtools/client/netmonitor/src/components/toolbar.js

it's caused by using `store.dispatch` in code. You can wrap this line of function in connect section, like ``` disableBrowserCache: (disabled) => dispatch(Actions.disableBrowserCache(disabled)), ``` and call disableBrowserCache(Services.prefs.getBoolPref....) in the origin place

Comment on attachment 8885221 [details] Bug 1349561 - Add UI for disabling browser cache in Net panel; Sorry for the multiple mozreview requests. The suggestion in comment#30 worked. Thanks! I've given a Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a73504f104baac45e44420973606f95ff74c5607

Comment on attachment 8885221 [details] Bug 1349561 - Add UI for disabling browser cache in Net panel; https://reviewboard.mozilla.org/r/156106/#review161492 test on delive and it works well. Please remove the ".devtools-checkbox" className for input and it will be ready to land ::: devtools/client/netmonitor/src/assets/styles/netmonitor.css:790 (Diff revision 1) > > .properties-view .devtools-searchbox input { > margin: 1px 3px; > } > > +.devtools-checkbox { seems we don't need that ::: devtools/client/netmonitor/src/components/toolbar.js:129 (Diff revision 1) > + { > + className: "devtools-checkbox-label", > + title: L10N.getStr("netmonitor.toolbar.disableCache.tooltip"), > + }, > + input({ > + className: "devtools-checkbox", add this class make the checkbop a bit lower than the related string, please remove this class.

Comment on attachment 8885221 [details] Bug 1349561 - Add UI for disabling browser cache in Net panel; https://reviewboard.mozilla.org/r/156106/#review161494

Comment on attachment 8885221 [details] Bug 1349561 - Add UI for disabling browser cache in Net panel; Shall I give another run on Try?

Comment on attachment 8885221 [details] Bug 1349561 - Add UI for disabling browser cache in Net panel; https://reviewboard.mozilla.org/r/156106/#review161568 looks good to me, thanks Swapnesh!

I think its fine since the latest change is only remove styles. Thank you for your patient and the willing to bring it into reality.

need final review/ship from a suitable reviewer on mozreview. Can you take a look at this, so that we can use autoland ? Thanks!

Comment on attachment 8885221 [details] Bug 1349561 - Add UI for disabling browser cache in Net panel; Honza, seems I did not have the module permission, could you help review this as well?

Comment on attachment 8885221 [details] Bug 1349561 - Add UI for disabling browser cache in Net panel; https://reviewboard.mozilla.org/r/156106/#review162880 Thanks for working on this Swapnesh, Fred! Two comments related to UI/UX * The checkbox should be displayed next to the list of filter buttons * The checkbox and assocated label should be properly vertically centered. (I'll attach commented screenshot) Honza

Comment on attachment 8885221 [details] Bug 1349561 - Add UI for disabling browser cache in Net panel; https://reviewboard.mozilla.org/r/156106/#review164100 Excellent! R+, but please increase the gap between filters and the checkbox as follow: .devtools-checkbox-label { margin-inline-start: 10px; <---- 10px margin-inline-end: 3px; } Thanks! Honza

Pushed by jodvarko@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/f026b16bb1e8 Add UI for disabling browser cache in Net panel; r=gasolin,Honza

Thanks Fred, Honza for your guidance. It was fun learning Redux. :)

Thank you for working on this issue! Honza

Thank you Swapnesh to bring this very handy feature!

Great feature! Screenshots: https://screenshots.mattn.ca/compare/?oldProject=mozilla-central&oldRev=7d2e89fb92331d7e4296391213c1e63db628e046&newProject=mozilla-central&newRev=0faada5c2f308f101ab7c54a87b3dce80b97d0e3&filter=netmonitor Note that this very slightly increased the row size on OSX and Linux, not sure if we care.

Hey Swapnesh, Did you work with devtools-launchpad while developing this fix? Strangely I'm working on a bug which refuses to launch using `yarn start`. Can you look at bug#1364096 comment#43 ? Not sure if your fix is linked with this issue that I'm facing. I'm attaching patch which u can apply over latest master to understand the problem. Thanks, Ruturaj

The new pref needs to be added here as well: https://dxr.mozilla.org/mozilla-central/source/devtools/client/netmonitor/index.js Ruturaj or Swapnesh, do you think you could upload a new patch addressing that ?

ya, I'll do it. Is it ok if I add it in bug#1364096 ? Or you want another bug/patch?

I've filed bug 1384033 for this.

(In reply to Ruturaj Vartak from comment #52) > Did you work with devtools-launchpad while developing this fix? Actually, I worked with netmonitor and not launchpad while developing the fix. Thanks for taking up the work on the fix. Let me know if my help is needed anywhere.

I can confirm that this feature works as expected on Nightly 56.a01 across platforms: Windows 10 x64, Mac OS X 10.11 and Ubuntu 16.04 x64 LTS. Marking this issue verified fixed.