Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2860543002:
[Offline pages] Fix order of error checking so URL mismatch would not override other error codes (Closed)
|
Created:
3 years, 7 months ago by chili Modified:
3 years, 7 months ago CC:
chromium-reviews, romax+watch_chromium.org, fgorski+watch_chromium.org, dewittj+watch_chromium.org, petewil+watch_chromium.org, chili+watch_chromium.org, dimich+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[Offline pages] Fix order of error checking so URL mismatch would not override other error codes
BUG=717706
Review-Url: https://codereview.chromium.org/2860543002
Cr-Commit-Position: refs/heads/master@{#469082}
Committed: https://chromium.googlesource.com/chromium/src/+/7ed8f251fa9ae767f20a32abe8d70921b883d062
Patch Set 1 #Patch Set 2 : test #Patch Set 3 : change order of if-blocks #Patch Set 4 : rename url to something else #
Messages
Total messages: 21 (12 generated)
chili@chromium.org changed reviewers: + carlosk@chromium.org, petewil@chromium.org
Have you considered the option to change the order of the if blocks in OfflinePageModelImpl::OnCreateArchiveDone? If the error check should happen before the URL check, wouldn't that already solve the issue?
On 2017/05/02 21:24:28, carlosk wrote: > Have you considered the option to change the order of the if blocks in > OfflinePageModelImpl::OnCreateArchiveDone? If the error check should happen > before the URL check, wouldn't that already solve the issue? Yes I've thought about it. Decided to go this route because: 1. I don't see a particular advantage doing it one way or the other 2. Given other things equal, I prefer not to change order of code - gives freedom to change ordering later on if a better reason arises That said, #1 is I don't really care either way (other than laziness since I've already gone this route =D). Do you feel a strong preference for changing order of if-blocks?
lgtm (once CarlosK's concerns are all addressed)
The CQ bit was checked by chili@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...
On 2017/05/02 21:45:20, chili wrote: > On 2017/05/02 21:24:28, carlosk wrote: > > Have you considered the option to change the order of the if blocks in > > OfflinePageModelImpl::OnCreateArchiveDone? If the error check should happen > > before the URL check, wouldn't that already solve the issue? > > Yes I've thought about it. Decided to go this route because: > 1. I don't see a particular advantage doing it one way or the other > 2. Given other things equal, I prefer not to change order of code - gives > freedom to change ordering later on if a better reason arises > > That said, #1 is I don't really care either way (other than laziness since I've > already gone this route =D). Do you feel a strong preference for changing order > of if-blocks? We are giving higher priority to reporting the URL mismatch error even if another error caused the failure in the first place and this seems incorrect. Also it seems to me that the intent of that final URL check is to verify that a successfully generated page matches what was originally requested. That's why the WebContents passes the URL that was actually offlined to the callback received by GenerateMHTML (OfflinePageMHTMLArchiver::OnGenerateMHTMLDone in this case). However for most failure cases being handled by OfflinePageMHTMLArchiver::ReportFailure it seems that providing an "actual URL" doesn't make sense because no page was offlined. It might make sense when the WebContents itself reports the failure but in that case we should use the URL that it passed to OnGenerateMHTMLDone. That's all to say that apparently I feel strongly about it. :) My suggestion to solve this issue is: - Change the order of those if-checks to prioritize reporting an actual failure independently of URL mismatches. - Rename misleading "url" parameter of OfflinePageModelImpl::OnCreateArchiveDone to "actual_url" or "final_url" or something in those lines. - Optional: If we want/need to report a URL for some/all failure cases then change OfflinePageMHTMLArchiver::ReportFailure to accept a URL from its callers and pass that one on instead. WDYT?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by chili@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...
On 2017/05/02 22:36:04, carlosk wrote: > On 2017/05/02 21:45:20, chili wrote: > > On 2017/05/02 21:24:28, carlosk wrote: > > > Have you considered the option to change the order of the if blocks in > > > OfflinePageModelImpl::OnCreateArchiveDone? If the error check should happen > > > before the URL check, wouldn't that already solve the issue? > > > > Yes I've thought about it. Decided to go this route because: > > 1. I don't see a particular advantage doing it one way or the other > > 2. Given other things equal, I prefer not to change order of code - gives > > freedom to change ordering later on if a better reason arises > > > > That said, #1 is I don't really care either way (other than laziness since > I've > > already gone this route =D). Do you feel a strong preference for changing > order > > of if-blocks? > > We are giving higher priority to reporting the URL mismatch error even if > another error caused the failure in the first place and this seems incorrect. > > Also it seems to me that the intent of that final URL check is to verify that a > successfully generated page matches what was originally requested. That's why > the WebContents passes the URL that was actually offlined to the callback > received by GenerateMHTML (OfflinePageMHTMLArchiver::OnGenerateMHTMLDone in this > case). > > However for most failure cases being handled by > OfflinePageMHTMLArchiver::ReportFailure it seems that providing an "actual URL" > doesn't make sense because no page was offlined. It might make sense when the > WebContents itself reports the failure but in that case we should use the URL > that it passed to OnGenerateMHTMLDone. > > That's all to say that apparently I feel strongly about it. :) My suggestion to > solve this issue is: > - Change the order of those if-checks to prioritize reporting an actual failure > independently of URL mismatches. > - Rename misleading "url" parameter of OfflinePageModelImpl::OnCreateArchiveDone > to "actual_url" or "final_url" or something in those lines. > - Optional: If we want/need to report a URL for some/all failure cases then > change OfflinePageMHTMLArchiver::ReportFailure to accept a URL from its callers > and pass that one on instead. > > WDYT? Done
Thanks and LGTM.
The CQ bit was checked by chili@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from petewil@chromium.org Link to the patchset: https://codereview.chromium.org/2860543002/#ps60001 (title: "rename url to something else")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [Offline pages] Fix mhtml archiver to return a reasonable URL when reporting failure BUG=717706 ========== to ========== [Offline pages] Fix order of error checking so URL mismatch would not override other error codes BUG=717706 ==========
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1493833894081420,
"parent_rev": "f4335e87b019d74e3d803318101922bb40cec682", "commit_rev":
"7ed8f251fa9ae767f20a32abe8d70921b883d062"}
Message was sent while issue was closed.
Description was changed from ========== [Offline pages] Fix order of error checking so URL mismatch would not override other error codes BUG=717706 ========== to ========== [Offline pages] Fix order of error checking so URL mismatch would not override other error codes BUG=717706 Review-Url: https://codereview.chromium.org/2860543002 Cr-Commit-Position: refs/heads/master@{#469082} Committed: https://chromium.googlesource.com/chromium/src/+/7ed8f251fa9ae767f20a32abe8d7... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/7ed8f251fa9ae767f20a32abe8d7... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
