Closed Bug 1404929 Opened 3 years ago Closed 3 years ago

Security info should be loaded lazily

Categories

(DevTools :: Netmonitor, enhancement, P2)

enhancement

Tracking

(firefox57 fix-optional, firefox59 fixed)

RESOLVED FIXED
Firefox 59
Tracking Status
firefox57 --- fix-optional
firefox59 --- fixed

People

(Reporter: ochameau, Assigned: gasolin)

References

Details

Attachments

(1 file)

Per bug 1404913, we should try to lazy load security info

It is currently used from:
* SecurityPanel
http://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/components/security-panel.js#31
test without getRequestData for security info, get good result

complicated.netmonitor.requestsFinished 9,711.77  > 9,382 -3.39% (high)
simple.netmonitor.requestsFinished 183.89 > 172.94 -5.95% (med)

https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=mozilla-central&newProject=try&newRevision=7a821499ed8fc9290b427f7c5fb966e1fbf28b83&originalSignature=edaec66500db21d37602c99daa61ac983f21a6ac&newSignature=edaec66500db21d37602c99daa61ac983f21a6ac&filter=netmonitor&framework=1&selectedTimeRange=172800

I'd take a look Bug 1404917 and figure out how to do this right
Assignee: nobody → gasolin
Status: NEW → ASSIGNED
Duplicate of this bug: 1415433
Comparing to last X days on PerfHerder will report false result, as your changeset will include responseContent laziness and "the last X days" won't. So your results include bug 1404917 improvements...

I pushed to try bug 1404917 just before landing.
Try changeset was: f71c357ac4fe5b9d30780b61f9200b78a073609e

So for the next 2/3 days you would be safer comparing againt it:
https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=f71c357ac4fe5b9d30780b61f9200b78a073609e&newProject=try&newRevision=d0363c9fcafad98a6be5396e679fc21127f870a5&originalSignature=edaec66500db21d37602c99daa61ac983f21a6ac&newSignature=edaec66500db21d37602c99daa61ac983f21a6ac&showOnlyImportant=1&framework=1
The result is still looking good:
 complicated.netmonitor.requestsFinished  -4.73% 
 simple.netmonitor.requestsFinished -13.10%
Depends on: 1404917
Comment on attachment 8928395 [details]
Bug 1404929 - Security info should be loaded lazily;

https://reviewboard.mozilla.org/r/199604/#review206298

::: devtools/client/netmonitor/src/components/SecurityPanel.js:177
(Diff revision 5)
> -SecurityPanel.propTypes = {
> -  request: PropTypes.object.isRequired,
> -  openLink: PropTypes.func,
> -};
> -
>  function renderValue(props, weaknessReasons = []) {

After converting SecurityPanel to stateful react component, we can move those outside functions like `renderValue` into component itself.

::: devtools/client/netmonitor/test/browser_net_security-details.js:26
(Diff revision 5)
>      content.wrappedJSObject.performRequests(1, url);
>    });
>    yield wait;
>  
>    wait = waitForDOM(document, "#security-panel");
> +  let onSecurityInfo = monitor.panelWin.once(EVENTS.RECEIVED_SECURITY_INFO);

I don't prefer to rely on `EVENTS.RECEIVED_SECURITY_INFO` network event for tesitng. Emit events in source code but only used in writing tests that is not a good pattern IMO.

Let's try to add more specific selector rules in waitForDOM to wait until specific element rendering on document.
And then we could get rid of waitForTick().

waitForDOM can wait for specific condition with given number of elements. Please modify the rest of your tests as well.

https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/test/browser_net_complex-params.js#27
Comment on attachment 8928395 [details]
Bug 1404929 - Security info should be loaded lazily;

https://reviewboard.mozilla.org/r/199604/#review206342

Please resolve Ricky's comments and I can re-review.

Thanks Fred for the patch!
Honza

::: devtools/client/netmonitor/src/components/TabboxPanel.js:99
(Diff revision 5)
>        request.securityState && request.securityState !== "insecure" &&
>        TabPanel({
>          id: PANELS.SECURITY,
>          title: SECURITY_TITLE,
>        },
> -        SecurityPanel({ request, openLink }),
> +        SecurityPanel({ connector, openLink, request }),

nit: put arguments into separate lines
Attachment #8928395 - Flags: review?(odvarko)
Also, there is conflict with patch in Bug 1404928.
So, it might needs rebase in case Bug 1404928 lands first.

Honza
Comment on attachment 8928395 [details]
Bug 1404929 - Security info should be loaded lazily;

https://reviewboard.mozilla.org/r/199604/#review206298

> I don't prefer to rely on `EVENTS.RECEIVED_SECURITY_INFO` network event for tesitng. Emit events in source code but only used in writing tests that is not a good pattern IMO.
> 
> Let's try to add more specific selector rules in waitForDOM to wait until specific element rendering on document.
> And then we could get rid of waitForTick().
> 
> waitForDOM can wait for specific condition with given number of elements. Please modify the rest of your tests as well.
> 
> https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/test/browser_net_complex-params.js#27

removed onSecurityInfo in test cases and use `waitUntil` to listen for the UI change
Comment on attachment 8928395 [details]
Bug 1404929 - Security info should be loaded lazily;

https://reviewboard.mozilla.org/r/199604/#review207392

Looks good to me, thanks Fred!

R+ assuming try is gree and my two little comments resolved.

Honza

::: devtools/client/netmonitor/src/components/SecurityPanel.js:64
(Diff revision 6)
> +    };
> +  }
> +
> +  constructor(props) {
> +    super(props);
> +  }

We don't need the constructor, please remove.

::: devtools/client/netmonitor/src/components/SecurityPanel.js:81
(Diff revision 6)
> +   */
> +  componentWillReceiveProps(nextProps) {
> +    this.maybeFetchSecurityInfo(nextProps);
> +  }
> +
> +   /**

nit: the comment has wrong indentation
Attachment #8928395 - Flags: review?(odvarko) → review+
Comment on attachment 8928395 [details]
Bug 1404929 - Security info should be loaded lazily;

https://reviewboard.mozilla.org/r/199604/#review207392

> We don't need the constructor, please remove.

removed

> nit: the comment has wrong indentation

indentation changed
Pushed by flin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4bdad977319a
Security info should be loaded lazily;r=Honza
https://hg.mozilla.org/mozilla-central/rev/4bdad977319a
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.