Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2871303002:
Add missing fields to ResourceResponse::DeepCopy (Closed)
DescriptionAdd missing fields to ResourceResponse::DeepCopy
ResourceResponse::DeepCopy wasn't copying all fields, which, in the case of
security details, was causing PlzNavigate navigation requests to be missing
security info when committed.
A test will be added in a follow-up CL.
https: //horo-t.github.io. Open DevTools and go to the Security tab. Reload the
Review-Url: https://codereview.chromium.org/2871303002
Cr-Commit-Position: refs/heads/master@{#470752}
Committed: https://chromium.googlesource.com/chromium/src/+/bb92571ebde21cea713ce66c5330f5907ae5cafc
Patch Set 1 #Patch Set 2 : fix typo #Patch Set 3 : add another note about updating DeepCopy #
Messages
Total messages: 23 (14 generated)
The CQ bit was checked by estark@chromium.org to run a CQ dry run
estark@chromium.org changed reviewers: + nasko@chromium.org
nasko: PTAL? Will look into writing a browser test this afternoon.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by estark@chromium.org to run a CQ dry run
Oops, fixed a typo, now it actually compiles :)
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Maybe worthwhile putting a big fat comment in resource_response_info.h to point that whenever a new member is added the DeepCopy method needs to be updated. I was wondering why we missed these fields and it turns out that they were added when we refactored the certs code to remove the cert ids and store. So in that work we were oblivious to the requirement to add them to that method.
On 2017/05/10 19:51:32, nasko wrote: > Maybe worthwhile putting a big fat comment in resource_response_info.h to point > that whenever a new member is added the DeepCopy method needs to be updated. I > was wondering why we missed these fields and it turns out that they were added > when we refactored the certs code to remove the cert ids and store. So in that > work we were oblivious to the requirement to add them to that method. Heh, I went to go do this and then realized there already is a rather fat comment already :( https://cs.chromium.org/chromium/src/content/public/common/resource_response_... I could repeat it at the bottom of the file to increase the chances that someone sees it, but that seems a little much, WDYT?
On 2017/05/10 20:03:45, estark wrote: > On 2017/05/10 19:51:32, nasko wrote: > > Maybe worthwhile putting a big fat comment in resource_response_info.h to > point > > that whenever a new member is added the DeepCopy method needs to be updated. I > > was wondering why we missed these fields and it turns out that they were added > > when we refactored the certs code to remove the cert ids and store. So in that > > work we were oblivious to the requirement to add them to that method. > > Heh, I went to go do this and then realized there already is a rather fat > comment already :( > > https://cs.chromium.org/chromium/src/content/public/common/resource_response_... > > I could repeat it at the bottom of the file to increase the chances that someone > sees it, but that seems a little much, WDYT? It is a one liner, so I wouldn't call it fat : ). However, putting another pointer at the bottom is probably a good idea. If both of these are there and we still miss it ... oh well. But the probability to add new members at the bottom is high, so I'd say let's add one.
The CQ bit was checked by estark@chromium.org to run a CQ dry run
On 2017/05/10 20:08:39, nasko wrote: > On 2017/05/10 20:03:45, estark wrote: > > On 2017/05/10 19:51:32, nasko wrote: > > > Maybe worthwhile putting a big fat comment in resource_response_info.h to > > point > > > that whenever a new member is added the DeepCopy method needs to be updated. > I > > > was wondering why we missed these fields and it turns out that they were > added > > > when we refactored the certs code to remove the cert ids and store. So in > that > > > work we were oblivious to the requirement to add them to that method. > > > > Heh, I went to go do this and then realized there already is a rather fat > > comment already :( > > > > > https://cs.chromium.org/chromium/src/content/public/common/resource_response_... > > > > I could repeat it at the bottom of the file to increase the chances that > someone > > sees it, but that seems a little much, WDYT? > > It is a one liner, so I wouldn't call it fat : ). However, putting another > pointer at the bottom is probably a good idea. If both of these are there and we > still miss it ... oh well. But the probability to add new members at the bottom > is high, so I'd say let's add one. Ok, done. Also added some capslock for good measure :)
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
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_...)
LGTM and thanks a ton for the quick fix!
The CQ bit was checked by nasko@chromium.org
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": 40001, "attempt_start_ts": 1494454604934480,
"parent_rev": "1490e239dff73cdc914b39175d41df58d646e07f", "commit_rev":
"bb92571ebde21cea713ce66c5330f5907ae5cafc"}
Message was sent while issue was closed.
Description was changed from ========== Add missing fields to ResourceResponse::DeepCopy ResourceResponse::DeepCopy wasn't copying all fields, which, in the case of security details, was causing PlzNavigate navigation requests to be missing security info when committed. A test will be added in a follow-up CL. BUG=713203 TEST=Run Chrome with --enable-features=browser-side-navigation. Navigate to https://horo-t.github.io. Open DevTools and go to the Security tab. Reload the page. Observe a green dot next to "horo-t.github.io" underneath "Main Origin" in the Security Panel sidebar. ========== to ========== Add missing fields to ResourceResponse::DeepCopy ResourceResponse::DeepCopy wasn't copying all fields, which, in the case of security details, was causing PlzNavigate navigation requests to be missing security info when committed. A test will be added in a follow-up CL. https: //horo-t.github.io. Open DevTools and go to the Security tab. Reload the Review-Url: https://codereview.chromium.org/2871303002 Cr-Commit-Position: refs/heads/master@{#470752} Committed: https://chromium.googlesource.com/chromium/src/+/bb92571ebde21cea713ce66c5330... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/bb92571ebde21cea713ce66c5330... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
