Closed Bug 1349561 Opened 3 years ago Closed 2 years ago

Introduce UI for disabling browser cache in the Net panel

Categories

(DevTools :: Netmonitor, enhancement, P2)

enhancement

Tracking

(firefox56 verified)

VERIFIED FIXED
Firefox 56
Tracking Status
firefox56 --- verified

People

(Reporter: Honza, Assigned: swapneshks, Mentored)

References

Details

(Keywords: dev-doc-needed, good-first-bug, Whiteboard: [netmonitor])

Attachments

(3 files, 2 obsolete files)

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
Priority: -- → P2
Hi Honza, should this be added to LaunchPad?
Flags: needinfo?(odvarko)
(In reply to Marco Mucci [:MarcoM] from comment #1)
> Hi Honza, should this be added to LaunchPad?
yes

Thanks!

Honza
Flags: needinfo?(odvarko)
Flags: qe-verify?
Whiteboard: [netmonitor]
Flags: qe-verify? → qe-verify+
QA Contact: ciprian.georgiu
Since bug 1350228 is done, we can bring `disabling browser cache` checkbox in netmonitor
Mentor: gasolin
Depends on: 1350228
Keywords: good-first-bug
Hi.. Can I take up this bug?
I am a beginner (have worked on 6-7 moz bugs only).
Flags: needinfo?(gasolin)
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?
Assignee: nobody → swapneshks
Flags: needinfo?(gasolin) → needinfo?(odvarko)
(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
Flags: needinfo?(odvarko)
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. :)
Flags: needinfo?(gasolin)
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
Flags: needinfo?(gasolin)
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);
```
Attachment #8870533 - Flags: review?(gasolin)
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?
Flags: needinfo?(gasolin)
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}
Attachment #8870533 - Flags: review?(gasolin)
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)
Attachment #8870533 - Flags: review?(gasolin)
Flags: needinfo?(gasolin)
(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.
Flags: needinfo?(gasolin)
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.
Flags: needinfo?(gasolin)
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.
Flags: needinfo?(gasolin)
Attachment #8870533 - Attachment is obsolete: true
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
Flags: needinfo?(gasolin) → needinfo?(swapneshks)
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
Attachment #8880095 - Flags: review?(gasolin)
(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?
Flags: needinfo?(swapneshks) → needinfo?(gasolin)
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
Flags: needinfo?(gasolin)
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
Attachment #8885221 - Flags: review?(gasolin)
Comment on attachment 8885221 [details]
Bug 1349561 - Add UI for disabling browser cache in Net panel;

Shall I give another run on Try?
Flags: needinfo?(gasolin)
Attachment #8880095 - Attachment is obsolete: true
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!
Attachment #8885221 - Flags: review?(gasolin) → review+
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.
Flags: needinfo?(gasolin)
Keywords: checkin-needed
need final review/ship from a suitable reviewer on mozreview. Can you take a look at this, so that we can use autoland ? Thanks!
Flags: needinfo?(gasolin)
Keywords: checkin-needed
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?
Flags: needinfo?(gasolin)
Attachment #8885221 - Flags: review?(odvarko)
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
Attachment #8885221 - Flags: review?(odvarko) → review-
Status: NEW → ASSIGNED
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
Attachment #8885221 - Flags: review?(odvarko) → review+
Keywords: checkin-needed
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f026b16bb1e8
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
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!
Keywords: dev-doc-needed
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
Flags: needinfo?(swapneshks)
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?
Flags: needinfo?(swapneshks) → needinfo?(ntim.bugs)
Depends on: 1384033
I've filed bug 1384033 for this.
Flags: needinfo?(ntim.bugs)
(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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+ → qe-verify-
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.