1404929 - Security info should be loaded lazily

Status: RESOLVED FIXED

(DevTools :: Netmonitor, enhancement, P2)

Description

[Alexandre Poirot [:ochameau]]

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

Comment 1

[[e-mail]]

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

Comment 2

[[e-mail]]

Attachment: Bug 1404929 - Security info should be loaded lazily;

Review commit: https://reviewboard.mozilla.org/r/199604/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/199604/

Comment 4

[[e-mail]]

the patch shows very good result in DAMP

complicated.netmonitor.requestsFinished 9497 -> 7902 -16.80% (med)
simple.netmonitor.requestsFinished 191 -> 167 -12.57% (low)

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

will fix tests then send the review

Comment 5

[Alexandre Poirot [:ochameau]]

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%

Comment 6

[[e-mail]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/199604/diff/1-2/

Comment 7

[[e-mail]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/199604/diff/2-3/

Comment 8

[[e-mail]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/199604/diff/3-4/

Comment 9

[[e-mail]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/199604/diff/4-5/

Comment 10

[Ricky Chien [:rickychien] (inactive)]

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 11

[Jan Honza Odvarko [:Honza] (always need-info? me)]

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

Comment 12

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Also, there is conflict with patch in Bug 1404928.
So, it might needs rebase in case Bug 1404928 lands first.

Honza

Comment 13

[[e-mail]]

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 14

[[e-mail]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/199604/diff/5-6/

Comment 15

[Jan Honza Odvarko [:Honza] (always need-info? me)]

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

Comment 16

[[e-mail]]

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

Comment 17

[[e-mail]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/199604/diff/6-7/

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4bdad977319a
Security info should be loaded lazily;r=Honza

Comment 19

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/4bdad977319a