Closed Bug 1340368 Opened 7 years ago Closed 7 years ago

Rewrite har-exporter and use WebAPIs instead

Categories

(DevTools :: Netmonitor, defect, P1)

defect

Tracking

(firefox54 verified)

VERIFIED FIXED
Firefox 54
Iteration:
54.3 - Mar 6
Tracking Status
firefox54 --- verified

People

(Reporter: rickychien, Assigned: rickychien)

References

(Blocks 1 open bug)

Details

(Whiteboard: [netmonitor-reserve])

Attachments

(1 file)

Get rid of all XPCOM privilege APIs in order to make NetMonitor possible to run on content process.

* Remove har-utils dependency and rewrite read / write part through WebAPIs solution.
* Leave hat-utils.js as it is which will only being used by har-automation.
Flags: qe-verify?
Our de-chrome strategic will make all UI relevant actions invoke APIs from har-exporter but unable to access to har-utils anymore. Therefore, har-utils will only be used by har-automation so that we don't have to remove privilege APIs for har-automation and har-utils 

har-automation is used only for test automation like Selenium to record and save HAR log into disk. This feature needs to be enabled behind prefs which also needs so many Firefox privilege features.

My patch will copy getHarFileName() and formatDate() APIs from har-utils in order to get correct file name. As a result, you're going to see duplicate code (for above two APIs) between har-utils and har-exporter.

One big issue for us is whether we should keep zip file support. After enabling it from a pref "devtools.netmonitor.har.compress", you can download HAR as a compass zip format. However, I'm thinking that it's not so helpful to compass HAR log into a zip. If we would like to support this, we have to import some libraries like JSZip [1].

[1] https://stuk.github.io/jszip/
Ask for first round review. 

Note that my patch hasn't support zip file download yet. And there is a download.jsm error in console even though everything works fine.

This patch depends on bug 1340366, please the patch in bug 1340366 first. Thanks!
Flags: qe-verify? → qe-verify+
Priority: -- → P1
After discussion in meeting, our decision is to keep supporting all feature as possible. So, we will import JSZip library in devtools/shared.
Iteration: --- → 54.2 - Feb 20
QA Contact: ciprian.georgiu
Whiteboard: [netmonitor][triage] → [netmonitor-reserve]
Patch has updated with JSZip support. JSZip source code was placed in devtools/client/shared but not sure it's the right place for it.
I have no idea why console always appears an error after clicking "Save All As HAR".

[Exception... "Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIAnnotationService.setPageAnnotation]"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: resource:///modules/DownloadsCommon.jsm :: onDownloadChanged :: line 800"  data: no]

But everything works fine.
Iteration: 54.2 - Feb 20 → 54.3 - Mar 6
(In reply to Ricky Chien [:rickychien] from comment #8)
> I have no idea why console always appears an error after clicking "Save All
> As HAR".
> 
> [Exception... "Component returned failure code: 0x80070057
> (NS_ERROR_ILLEGAL_VALUE) [nsIAnnotationService.setPageAnnotation]" 
> nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame ::
> resource:///modules/DownloadsCommon.jsm :: onDownloadChanged :: line 800" 
> data: no]
> 
> But everything works fine.

My code copied this example from jsfiddle http://jsfiddle.net/koldev/cW7W5/.
As you can see, downloading file from example also throw the same error in browser toolbox console. It might tell us that error message comes from our internal APIs (DownloadsCommon.jsm), but it's harmless.

Patch is ready to go once JSZip licensing signoff in bug 1340469 is approved.
Depends on: 1340469
Comment on attachment 8838418 [details]
Bug 1340368 - Rewrite har-exporter and use WebAPIs instead

https://reviewboard.mozilla.org/r/113356/#review117448

The patch looks good and also works for me!

I like how it's done but, I think we should support standard "Save As" dialog in case the Panel is running inside real toolbox and use the Download manager only if we run inside a web page. This way we'll be consistent with other parts of DevTools (e.g. JSON Viewer is using "Save As" dialog too) and also using "Save As" dialog is simpler UX. 

What do you think?

We might want to discuss this at our meeting yet.

Honza
Attachment #8838418 - Flags: review?(odvarko)
Good suggestion! 

saveAs method undoubtedly should be a shared framework and leave in a shard folder. Patch has updated and I've moved saveAs to devtools/client/shared/file-saver.js for general purpose.
Comment on attachment 8838418 [details]
Bug 1340368 - Rewrite har-exporter and use WebAPIs instead

https://reviewboard.mozilla.org/r/113356/#review118190

R+ assuming try is green

Thanks Ricky!

Honza
Attachment #8838418 - Flags: review?(odvarko) → review+
Pushed by rchien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f7488b81a40a
Rewrite har-exporter and use WebAPIs instead r=Honza
https://hg.mozilla.org/mozilla-central/rev/f7488b81a40a
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
This issue is verified fixed on latest Aurora 54.0a2 (2017-03-06) on the following OSes:
- Windows 10 x64 
- Mac OS X 10.11.6. 

Marking here accordingly since the "Save All As HAR" feature is working as expected.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.