Chromium Code Reviews Chromium Code Reviews
Issue
1484093002:
Allowing multiple SaveItems to have same URLs. (Closed)
DescriptionAllowing multiple SaveItems to have same URLs.
Creating a SaveItem for each frame.
===================================
This CL causes SavePackage to create a separate SaveItem for each frame
(by splitting FindOrCreatePendingSaveItem method into CreatePendingSaveItem
and CreatePendingSaveItemDeduplicatingByUrl and using the latter only
for savable resources, but not for frames).
The change above means that URL is not anymore a good identifier of SaveItems
and consequently requires introducing save_item_id.
This code path is exercised by a new test in save_page_browsertest.cc that
saves a page with subframes that have the same URL, but different content.
At this point the test only ensures that we no longer crash in this scenario
- follow-up CLs are needed to actually preserve contents of the frames in this
scenario.
Using save_item_id instead of save_id and/or url.
=================================================
Before this CL, content::SaveItem didn't have its own id:
- In some places (i.e. SavePackage::SaveFailed) SaveItems were identified by
their URL. Using URL as an identifier is wrong (because 2 frames can have
the same URI, but their content can diverge at runtime - see the new
frames-runtime-changes.htm test being added in this CL).
- In other places (i.e. SavePackage::LookupItemInProcessBySaveId) SaveItems (*)
were identified by |save_id|. The problem with |save_id| was that it was
generated (with help of SaveFileManager::GetNextId)) *after* creation of
SaveItem and consequently there was a period when a SaveItem didn't have an
id. In addition to the need to use URLs as an id, this also meant having to
introduce SaveFileManager::RegisterStartingRequest to track requests that
don't yet have an id.
Note that |save_id| was used to identify more than SaveItems - it was also used
as an identifier for SaveFile and other things that SaveFileManager tracked,
but ultimately SaveItem is the first unique object associated with a file
writing request tracked by SaveFileManager, so we can use save_item_id in place
of save_id.
After this CL:
- content::SaveItem gets its own id (spelled save_item_id) that replaces
save_id + can be used instead of URL to identify a SaveItem.
Consequently:
- We no longer need SavePackage::SaveFailed and
SaveFileManager::OnErrorFinished (for the no-longer-applicable cases where
there is no save_id yet). In their place we can just use
SavePackage::SaveFinished and SaveFileManager::OnSaveFinished.
- We on longer need SaveFileManager::RegisterStartingRequest and other methods
types and fields used to maintain a map from (URL + contents_id) into
SavePackage. Now SaveFileManager::packages_ can always be used (because now
it uses always valid save_item_id, instead of lazily created save_id).
Other changes
=============
SavePackage should call SaveFileManager::SaveFinished once per frame (i.e. the
number_of_frames_pending_response == 0 would only be true if we already called
SaveFinished for all frames - therefore the 2nd call to SaveFinished is
redundant and can be removed). This ensures that all DCHECKs in
SaveFileManager work as expected.
BUG=106364
Committed: https://crrev.com/870cf11e7b1548dabca2c0deb62e346568ea4e8b
Cr-Commit-Position: refs/heads/master@{#364125}
Patch Set 1 #Patch Set 2 : Rebasing... #
Total comments: 15
Patch Set 3 : Rebasing... #
Total comments: 4
Patch Set 4 : Addressed CR feedback from Randy. #Patch Set 5 : Extra DCHECK + comment tweak. #Patch Set 6 : Rebasing... #Messages
Total messages: 16 (5 generated)
|
