Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2731293004:
Move the writing of the MHTML footer to the browser process. (Closed)
|
Created:
3 years, 9 months ago by carlosk Modified:
3 years, 9 months ago CC:
asanka, blink-reviews, blink-reviews-api_chromium.org, chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, dcheng, dglazkov+blink, jam, kinuko+watch, Łukasz Anforowicz, markusheintz_, mlamouri+watch-blink_chromium.org, mlamouri+watch-content_chromium.org, msramek+watch_chromium.org, nasko+codewatch_chromium.org, Pete Williamson, raymes+watch_chromium.org, jochen (gone - plz use gerrit) Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMove the writing of the MHTML footer to the browser process.
This change moves the MHTML generation step of writing the footer from
the renderer process to the browser process. Even though this spreads
MHTML code knowledge between Blink and content (a drawback) it does make
the generation process more robust and allows MHTML save operations to
become more robust to DOM changes.
The main issue we are facing with the Offline efforts is that frames can
be deleted after a MHTML save operation started and that will likely
cause it to fail completely. With this modification we can allow these
operations to optionally ignore missing frames.
I chose to still keep the footer code generation in MHTMLArchive but
make it explicit it is only used for testing purposes now. It seemed
more consistent and understandable for those tests to keep on generating
and reading fully valid MHTML files.
BUG=672292
TBR=jam@chromium.org
Review-Url: https://codereview.chromium.org/2731293004
Cr-Commit-Position: refs/heads/master@{#455935}
Committed: https://chromium.googlesource.com/chromium/src/+/7ae9f6fc18857892d7ffc3d5d3203720dd198c9f
Patch Set 1 #
Total comments: 11
Patch Set 2 : Comments and naming changes. #
Total comments: 15
Patch Set 3 : More minor changes #Patch Set 4 : More minor changes #Messages
Total messages: 32 (12 generated)
The CQ bit was checked by carlosk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
carlosk@chromium.org changed reviewers: + jochen@chromium.org, lukasza@chromium.org
tkent@: PTAL at Blink changes. nasko@: PTAL at content and IPC changes. lukasza@, petewil@: FYI. Note: Once I get LGTMs from you I will TBR jam@ for that include removal in chrome/browser and dcheng@ for the method name change in WebKit/Source/web/tests/. https://codereview.chromium.org/2731293004/diff/1/chrome/browser/content_sett... File chrome/browser/content_settings/content_settings_browsertest.cc (left): https://codereview.chromium.org/2731293004/diff/1/chrome/browser/content_sett... chrome/browser/content_settings/content_settings_browsertest.cc:40: #include "content/public/common/mhtml_generation_params.h" Removing this because it confused my codesearch for tests that touch MHTML: this include is currently unused.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
jochen@chromium.org changed reviewers: + nasko@chromium.org, tkent@chromium.org
adding the folks you asked to review the CL :)
On 2017/03/07 11:51:47, jochen wrote: > adding the folks you asked to review the CL :) Darn! And thanks! Indeed I think I missed some there... :)
third_party/WebKit lgtm
Mostly small things. https://codereview.chromium.org/2731293004/diff/1/content/browser/download/mh... File content/browser/download/mhtml_generation_manager.cc (right): https://codereview.chromium.org/2731293004/diff/1/content/browser/download/mh... content/browser/download/mhtml_generation_manager.cc:96: // The exact same boundary marker must to used for all "boundaries" -- header, nit: "must be used" https://codereview.chromium.org/2731293004/diff/1/content/browser/download/mh... content/browser/download/mhtml_generation_manager.cc:276: observed_renderer_process_host_.RemoveAll(); Why did this piece of code move? https://codereview.chromium.org/2731293004/diff/1/content/browser/download/mh... content/browser/download/mhtml_generation_manager.cc:343: save_status = MhtmlSaveStatus::FILE_WRITTING_ERROR; Without any comment, this looks very strange and wrong. If the file is invalid, why is the status even checked? https://codereview.chromium.org/2731293004/diff/1/content/browser/download/mh... content/browser/download/mhtml_generation_manager.cc:352: &MHTMLGenerationManager::Job::WriteFooterAndCloseFileOnFileThread, This name looks very long. Since marker is always written, why not keep it as a close file naming? The new design is that writing the marker is part of closing the file.
Thanks! https://codereview.chromium.org/2731293004/diff/1/content/browser/download/mh... File content/browser/download/mhtml_generation_manager.cc (right): https://codereview.chromium.org/2731293004/diff/1/content/browser/download/mh... content/browser/download/mhtml_generation_manager.cc:96: // The exact same boundary marker must to used for all "boundaries" -- header, On 2017/03/08 19:29:25, nasko (out March 7th) wrote: > nit: "must be used" Fixed that here and the mirror occurrences of this comment in MHTMLArchiver. Also slightly rephrased the comment. https://codereview.chromium.org/2731293004/diff/1/content/browser/download/mh... content/browser/download/mhtml_generation_manager.cc:276: observed_renderer_process_host_.RemoveAll(); On 2017/03/08 19:29:25, nasko (out March 7th) wrote: > Why did this piece of code move? I moved it so to bundle together all the actual logic in this method (non-metrics related). This (important) piece of code was less likely to be noticed where it was before (it happened to me). https://codereview.chromium.org/2731293004/diff/1/content/browser/download/mh... content/browser/download/mhtml_generation_manager.cc:343: save_status = MhtmlSaveStatus::FILE_WRITTING_ERROR; On 2017/03/08 19:29:25, nasko (out March 7th) wrote: > Without any comment, this looks very strange and wrong. If the file is invalid, > why is the status even checked? This is to not hide an earlier error in case one exists. I forgot to add a comment to clarify that but it has now been added. https://codereview.chromium.org/2731293004/diff/1/content/browser/download/mh... content/browser/download/mhtml_generation_manager.cc:352: &MHTMLGenerationManager::Job::WriteFooterAndCloseFileOnFileThread, On 2017/03/08 19:29:25, nasko (out March 7th) wrote: > This name looks very long. Since marker is always written, why not keep it as a > close file naming? The new design is that writing the marker is part of closing > the file. Done. Reverted back to old naming and kept the updated comments.
LGTM with a nit. https://codereview.chromium.org/2731293004/diff/1/content/browser/download/mh... File content/browser/download/mhtml_generation_manager.cc (right): https://codereview.chromium.org/2731293004/diff/1/content/browser/download/mh... content/browser/download/mhtml_generation_manager.cc:276: observed_renderer_process_host_.RemoveAll(); On 2017/03/08 21:45:54, carlosk wrote: > On 2017/03/08 19:29:25, nasko (out March 7th) wrote: > > Why did this piece of code move? > > I moved it so to bundle together all the actual logic in this method > (non-metrics related). This (important) piece of code was less likely to be > noticed where it was before (it happened to me). Keeping at least the TRACE macro towards the beginning of the method, before any code has executed seems more consistent with the rest of the codebase though.
Description was changed from ========== Move the writing of the MHTML footer to the browser process. This change moves the MHTML generation step of writing the footer from the renderer process to the browser process. Even though this spreads MHTML code knowledge between Blink and content (a drawback) it does make the generation process more robust and allows MHTML save operations to become more robust to DOM changes. The main issue we are facing with the Offline efforts is that frames can be deleted after a MHTML save operation started and that will likely cause it to fail completely. With this modification we can allow these operations to optionally ignore missing frames. I chose to still keep the footer code generation in MHTMLArchive but make it explicit it is only used for testing purposes now. It seemed more consistent and understandable for those tests to keep on generating and reading fully valid MHTML files. BUG=672292 ========== to ========== Move the writing of the MHTML footer to the browser process. This change moves the MHTML generation step of writing the footer from the renderer process to the browser process. Even though this spreads MHTML code knowledge between Blink and content (a drawback) it does make the generation process more robust and allows MHTML save operations to become more robust to DOM changes. The main issue we are facing with the Offline efforts is that frames can be deleted after a MHTML save operation started and that will likely cause it to fail completely. With this modification we can allow these operations to optionally ignore missing frames. I chose to still keep the footer code generation in MHTMLArchive but make it explicit it is only used for testing purposes now. It seemed more consistent and understandable for those tests to keep on generating and reading fully valid MHTML files. BUG=672292 TBR=jam@chromium.org ==========
carlosk@chromium.org changed reviewers: + dcheng@chromium.org, jam@chromium.org - jochen@chromium.org, lukasza@chromium.org
Thanks. dcheng@: PTAL. Also added jam@ for TBR. https://codereview.chromium.org/2731293004/diff/1/content/browser/download/mh... File content/browser/download/mhtml_generation_manager.cc (right): https://codereview.chromium.org/2731293004/diff/1/content/browser/download/mh... content/browser/download/mhtml_generation_manager.cc:276: observed_renderer_process_host_.RemoveAll(); On 2017/03/08 23:19:31, nasko (slow) wrote: > On 2017/03/08 21:45:54, carlosk wrote: > > On 2017/03/08 19:29:25, nasko (out March 7th) wrote: > > > Why did this piece of code move? > > > > I moved it so to bundle together all the actual logic in this method > > (non-metrics related). This (important) piece of code was less likely to be > > noticed where it was before (it happened to me). > > Keeping at least the TRACE macro towards the beginning of the method, before any > code has executed seems more consistent with the rest of the codebase though. I would agree with you for regular trace events. But this one is a) an instant event and b) was created to mark the very end of the job. So having it after that call is in fact ever so slightly more correct. :)
https://codereview.chromium.org/2731293004/diff/20001/content/browser/downloa... File content/browser/download/mhtml_generation_manager.cc (right): https://codereview.chromium.org/2731293004/diff/20001/content/browser/downloa... content/browser/download/mhtml_generation_manager.cc:81: base::Callback<void(std::tuple<MhtmlSaveStatus, int64_t>)> callback, Pass by const ref. Also, I'm not a content owner, but I would prefer a struct with explicitly named parameters rather than using a tuple. https://codereview.chromium.org/2731293004/diff/20001/content/browser/downloa... content/browser/download/mhtml_generation_manager.cc:102: std::string boundary, const std::string& https://codereview.chromium.org/2731293004/diff/20001/content/browser/downloa... content/browser/download/mhtml_generation_manager.cc:357: base::Passed(std::move(browser_file_))), Nit: base::Passed(&browser_file_) https://codereview.chromium.org/2731293004/diff/20001/third_party/WebKit/publ... File third_party/WebKit/public/web/WebFrameSerializer.h (left): https://codereview.chromium.org/2731293004/diff/20001/third_party/WebKit/publ... third_party/WebKit/public/web/WebFrameSerializer.h:104: const WebString& boundary); For symmetry, it would be nice to remove generateMHTMLHeader() from the public API as well. Would it be possible to do that in a followup?
lgtm
Thanks! https://codereview.chromium.org/2731293004/diff/20001/content/browser/downloa... File content/browser/download/mhtml_generation_manager.cc (right): https://codereview.chromium.org/2731293004/diff/20001/content/browser/downloa... content/browser/download/mhtml_generation_manager.cc:81: base::Callback<void(std::tuple<MhtmlSaveStatus, int64_t>)> callback, On 2017/03/09 01:56:51, dcheng wrote: > Pass by const ref. Also, I'm not a content owner, but I would prefer a struct > with explicitly named parameters rather than using a tuple. I did change to const ref. But these are just two integer values that I would normally pass as copies if they were regular parameters. And as this is just a class private exchange it seemed like an overkill to create a strut just to be able to return two parameters at once. If this was public or needed more than 2 values I would have switched to a struc (kind of my rule-of-thumb). https://codereview.chromium.org/2731293004/diff/20001/content/browser/downloa... content/browser/download/mhtml_generation_manager.cc:102: std::string boundary, On 2017/03/09 01:56:51, dcheng wrote: > const std::string& In this specific case the copy was intended. Between the time this call is scheduled and it is executed the Job itself can be destroyed what would create an invalid reference. That's also why I go the extra mile to send an empty string in case the boundary will not be used. https://codereview.chromium.org/2731293004/diff/20001/content/browser/downloa... content/browser/download/mhtml_generation_manager.cc:357: base::Passed(std::move(browser_file_))), On 2017/03/09 01:56:51, dcheng wrote: > Nit: base::Passed(&browser_file_) Done. https://codereview.chromium.org/2731293004/diff/20001/third_party/WebKit/publ... File third_party/WebKit/public/web/WebFrameSerializer.h (left): https://codereview.chromium.org/2731293004/diff/20001/third_party/WebKit/publ... third_party/WebKit/public/web/WebFrameSerializer.h:104: const WebString& boundary); On 2017/03/09 01:56:51, dcheng wrote: > For symmetry, it would be nice to remove generateMHTMLHeader() from the public > API as well. Would it be possible to do that in a followup? I am still considering it (you can see it mentioned in the referred issue). Even though I appreciate the symmetry aspect the header requires more data and more tooling on that I still have to figure out I can practically obtain/use from inside content/.
https://codereview.chromium.org/2731293004/diff/20001/content/browser/downloa... File content/browser/download/mhtml_generation_manager.cc (right): https://codereview.chromium.org/2731293004/diff/20001/content/browser/downloa... content/browser/download/mhtml_generation_manager.cc:102: std::string boundary, On 2017/03/09 22:03:36, carlosk wrote: > On 2017/03/09 01:56:51, dcheng wrote: > > const std::string& > > In this specific case the copy was intended. Between the time this call is > scheduled and it is executed the Job itself can be destroyed what would create > an invalid reference. That's also why I go the extra mile to send an empty > string in case the boundary will not be used. The default callback binding is always by copy, even if the parameter is a const ref parameter: https://chromium.googlesource.com/chromium/src/+/master/docs/callback.md#Pass... So you don't need to explicitly copy here, since it will be copied into callback's storage. (This is different from how some other callback systems work) https://codereview.chromium.org/2731293004/diff/20001/third_party/WebKit/publ... File third_party/WebKit/public/web/WebFrameSerializer.h (left): https://codereview.chromium.org/2731293004/diff/20001/third_party/WebKit/publ... third_party/WebKit/public/web/WebFrameSerializer.h:104: const WebString& boundary); On 2017/03/09 22:03:36, carlosk wrote: > On 2017/03/09 01:56:51, dcheng wrote: > > For symmetry, it would be nice to remove generateMHTMLHeader() from the public > > API as well. Would it be possible to do that in a followup? > > I am still considering it (you can see it mentioned in the referred issue). Even > though I appreciate the symmetry aspect the header requires more data and more > tooling on that I still have to figure out I can practically obtain/use from > inside content/. My concern is that the interaction between Blink and content for page serialization is already very complicated, and not having a consistent interface will be a long-term maintenance burden. The dependencies on title and mime type should both be satisfiable from content: WebDocument has a title getter, and the WebDataSource of the frame points to the response, which has an associated MIME type.
https://codereview.chromium.org/2731293004/diff/20001/content/browser/downloa... File content/browser/download/mhtml_generation_manager.cc (right): https://codereview.chromium.org/2731293004/diff/20001/content/browser/downloa... content/browser/download/mhtml_generation_manager.cc:102: std::string boundary, On 2017/03/09 22:08:46, dcheng wrote: > On 2017/03/09 22:03:36, carlosk wrote: > > On 2017/03/09 01:56:51, dcheng wrote: > > > const std::string& > > > > In this specific case the copy was intended. Between the time this call is > > scheduled and it is executed the Job itself can be destroyed what would create > > an invalid reference. That's also why I go the extra mile to send an empty > > string in case the boundary will not be used. > > The default callback binding is always by copy, even if the parameter is a const > ref parameter: > https://chromium.googlesource.com/chromium/src/+/master/docs/callback.md#Pass... > So you don't need to explicitly copy here, since it will be copied into > callback's storage. > > (This is different from how some other callback systems work) Understood and done. Thanks for the pointer. https://codereview.chromium.org/2731293004/diff/20001/third_party/WebKit/publ... File third_party/WebKit/public/web/WebFrameSerializer.h (left): https://codereview.chromium.org/2731293004/diff/20001/third_party/WebKit/publ... third_party/WebKit/public/web/WebFrameSerializer.h:104: const WebString& boundary); On 2017/03/09 22:08:46, dcheng wrote: > On 2017/03/09 22:03:36, carlosk wrote: > > On 2017/03/09 01:56:51, dcheng wrote: > > > For symmetry, it would be nice to remove generateMHTMLHeader() from the > public > > > API as well. Would it be possible to do that in a followup? > > > > I am still considering it (you can see it mentioned in the referred issue). > Even > > though I appreciate the symmetry aspect the header requires more data and more > > tooling on that I still have to figure out I can practically obtain/use from > > inside content/. > > My concern is that the interaction between Blink and content for page > serialization is already very complicated, and not having a consistent interface > will be a long-term maintenance burden. The dependencies on title and mime type > should both be satisfiable from content: WebDocument has a title getter, and the > WebDataSource of the frame points to the response, which has an associated MIME > type. Acknowledged.
https://codereview.chromium.org/2731293004/diff/20001/third_party/WebKit/publ... File third_party/WebKit/public/web/WebFrameSerializer.h (left): https://codereview.chromium.org/2731293004/diff/20001/third_party/WebKit/publ... third_party/WebKit/public/web/WebFrameSerializer.h:104: const WebString& boundary); On 2017/03/09 23:07:29, carlosk wrote: > On 2017/03/09 22:08:46, dcheng wrote: > > On 2017/03/09 22:03:36, carlosk wrote: > > > On 2017/03/09 01:56:51, dcheng wrote: > > > > For symmetry, it would be nice to remove generateMHTMLHeader() from the > > public > > > > API as well. Would it be possible to do that in a followup? > > > > > > I am still considering it (you can see it mentioned in the referred issue). > > Even > > > though I appreciate the symmetry aspect the header requires more data and > more > > > tooling on that I still have to figure out I can practically obtain/use from > > > inside content/. > > > > My concern is that the interaction between Blink and content for page > > serialization is already very complicated, and not having a consistent > interface > > will be a long-term maintenance burden. The dependencies on title and mime > type > > should both be satisfiable from content: WebDocument has a title getter, and > the > > WebDataSource of the frame points to the response, which has an associated > MIME > > type. > > Acknowledged. To clarify, does that mean you'll be investigating now or in a followup? If it's a followup, do you mind filing a bug and adding a TODO so we don't lose track of this?
https://codereview.chromium.org/2731293004/diff/20001/third_party/WebKit/publ... File third_party/WebKit/public/web/WebFrameSerializer.h (left): https://codereview.chromium.org/2731293004/diff/20001/third_party/WebKit/publ... third_party/WebKit/public/web/WebFrameSerializer.h:104: const WebString& boundary); On 2017/03/09 23:09:17, dcheng wrote: > On 2017/03/09 23:07:29, carlosk wrote: > > On 2017/03/09 22:08:46, dcheng wrote: > > > On 2017/03/09 22:03:36, carlosk wrote: > > > > On 2017/03/09 01:56:51, dcheng wrote: > > > > > For symmetry, it would be nice to remove generateMHTMLHeader() from the > > > public > > > > > API as well. Would it be possible to do that in a followup? > > > > > > > > I am still considering it (you can see it mentioned in the referred > issue). > > > Even > > > > though I appreciate the symmetry aspect the header requires more data and > > more > > > > tooling on that I still have to figure out I can practically obtain/use > from > > > > inside content/. > > > > > > My concern is that the interaction between Blink and content for page > > > serialization is already very complicated, and not having a consistent > > interface > > > will be a long-term maintenance burden. The dependencies on title and mime > > type > > > should both be satisfiable from content: WebDocument has a title getter, and > > the > > > WebDataSource of the frame points to the response, which has an associated > > MIME > > > type. > > > > Acknowledged. > > To clarify, does that mean you'll be investigating now or in a followup? If it's > a followup, do you mind filing a bug and adding a TODO so we don't lose track of > this? Not now. I will keep the linked issue (https://crbug.com/672292) open anyway until I look into that.
LGTM https://codereview.chromium.org/2731293004/diff/20001/content/browser/downloa... File content/browser/download/mhtml_generation_manager.cc (right): https://codereview.chromium.org/2731293004/diff/20001/content/browser/downloa... content/browser/download/mhtml_generation_manager.cc:81: base::Callback<void(std::tuple<MhtmlSaveStatus, int64_t>)> callback, On 2017/03/09 22:03:35, carlosk wrote: > On 2017/03/09 01:56:51, dcheng wrote: > > Pass by const ref. Also, I'm not a content owner, but I would prefer a struct > > with explicitly named parameters rather than using a tuple. > > I did change to const ref. But these are just two integer values that I would > normally pass as copies if they were regular parameters. > > And as this is just a class private exchange it seemed like an overkill to > create a strut just to be able to return two parameters at once. If this was > public or needed more than 2 values I would have switched to a struc (kind of my > rule-of-thumb). I would still prefer to just declare a simple local struct, for maintainability in the future. It's more robust to code changes in the future accidentally breaking things (granted, it's unlikely at the moment, since the parameters are different types, but it's a low-cost thing to do)
The CQ bit was checked by carlosk@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tkent@chromium.org, jam@chromium.org, nasko@chromium.org Link to the patchset: https://codereview.chromium.org/2731293004/#ps60001 (title: "More minor changes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1489101620879090,
"parent_rev": "0a2a68937880d75c5860d528cb788f14d46923f3", "commit_rev":
"7ae9f6fc18857892d7ffc3d5d3203720dd198c9f"}
Message was sent while issue was closed.
Description was changed from ========== Move the writing of the MHTML footer to the browser process. This change moves the MHTML generation step of writing the footer from the renderer process to the browser process. Even though this spreads MHTML code knowledge between Blink and content (a drawback) it does make the generation process more robust and allows MHTML save operations to become more robust to DOM changes. The main issue we are facing with the Offline efforts is that frames can be deleted after a MHTML save operation started and that will likely cause it to fail completely. With this modification we can allow these operations to optionally ignore missing frames. I chose to still keep the footer code generation in MHTMLArchive but make it explicit it is only used for testing purposes now. It seemed more consistent and understandable for those tests to keep on generating and reading fully valid MHTML files. BUG=672292 TBR=jam@chromium.org ========== to ========== Move the writing of the MHTML footer to the browser process. This change moves the MHTML generation step of writing the footer from the renderer process to the browser process. Even though this spreads MHTML code knowledge between Blink and content (a drawback) it does make the generation process more robust and allows MHTML save operations to become more robust to DOM changes. The main issue we are facing with the Offline efforts is that frames can be deleted after a MHTML save operation started and that will likely cause it to fail completely. With this modification we can allow these operations to optionally ignore missing frames. I chose to still keep the footer code generation in MHTMLArchive but make it explicit it is only used for testing purposes now. It seemed more consistent and understandable for those tests to keep on generating and reading fully valid MHTML files. BUG=672292 TBR=jam@chromium.org Review-Url: https://codereview.chromium.org/2731293004 Cr-Commit-Position: refs/heads/master@{#455935} Committed: https://chromium.googlesource.com/chromium/src/+/7ae9f6fc18857892d7ffc3d5d320... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/7ae9f6fc18857892d7ffc3d5d320...
Message was sent while issue was closed.
On 2017/03/10 01:00:33, commit-bot: I haz the power wrote: > Committed patchset #4 (id:60001) as > https://chromium.googlesource.com/chromium/src/+/7ae9f6fc18857892d7ffc3d5d320... bad solution: It better to have lock on frames to avoid deletion while MHTML save operation is running misuse of process isoltion
Message was sent while issue was closed.
On 2017/03/10 11:36:02, RE66 wrote: > On 2017/03/10 01:00:33, commit-bot: I haz the power wrote: > > Committed patchset #4 (id:60001) as > > > https://chromium.googlesource.com/chromium/src/+/7ae9f6fc18857892d7ffc3d5d320... > > bad solution: > It better to have lock on frames to avoid deletion while > MHTML save operation is running > > misuse of process isoltion Hi Ron. In this case we don't want to lock the frame tree/DOM because: a) this would cause a user-facing effect due to a potentially non user-requested actions. b) the saving can take a long time (median at 200ms but 90th-percentile is already above 1s). Could you elaborate on the misuse you mentioned? |
