Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2427633004:
Don't overwrite PaymentDetails (Closed)
|
Created:
4 years, 2 months ago by please use gerrit instead Modified:
4 years, 2 months ago CC:
chromium-reviews, blink-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDon't overwrite PaymentDetails
Overwriting shipping options in PaymentDetails causes a double-delete
due to the bug in Oilpan GC http://crbug.com/601193. A workaround until
that bug is fixed is to avoid overwriting shipping options in
PaymentDetails. However, shipping options cannot be sent to the browser
as-is. Therefore, the shipping options are overwritten in the Mojo data
structure instead.
BUG=600080, 601193
Committed: https://crrev.com/4d6514453bd381827b30e0e1fcb15fb211387a51
Cr-Commit-Position: refs/heads/master@{#426229}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Clarify the intent of the code. Add a TODO depending on GC fix. #Messages
Total messages: 27 (19 generated)
The CQ bit was checked by rouslan@chromium.org to run a CQ dry run
The CQ bit was checked by rouslan@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Description was changed from ========== Don't overwrite PaymentDetails BUG=600080, 601193 ========== to ========== Don't overwrite PaymentDetails Overwriting shipping options in PaymentDetails causes a double-delete due to the bug in Oilpan GC http://crbug.com/601193. A workaround until that bug is fixed is to avoid overwriting shipping options in PaymentDetails. However, shipping options cannot be sent to the browser as-is. Therefore, the shipping options are overwritten in the Mojo data structure instead. BUG=600080, 601193 ==========
The CQ bit was checked by rouslan@chromium.org to run a CQ dry run
Patchset #1 (id:1) has been deleted
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 checked by rouslan@chromium.org to run a CQ dry run
Patchset #1 (id:20001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
rouslan@chromium.org changed reviewers: + mathp@chromium.org
mathp@: PTAL. I own this code, but my co-owner is OOO, so I cannot send this patch to him.
lgtm. code is a bit weird but I got it. Perhaps we can reference a bug somewhere so that we come back to it once Oilpan fixes the HeapVector? https://codereview.chromium.org/2427633004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/payments/PaymentRequest.cpp (right): https://codereview.chromium.org/2427633004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:296: // exception. It took me a few passes to understand this pattern. Perhaps you could expand on the comment here or at the callsite. Also it's a bit jarring to see exceptions being thrown and "return true;" right after, but I understand the need...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
LGTM I'm sorry I haven't yet fixed the Oilpan bug... I'm traveling this week but will try to fix it next week.
Patchset #2 (id:60001) has been deleted
On 2016/10/18 18:28:22, haraken wrote: > I'm sorry I haven't yet fixed the Oilpan bug... I'm traveling this week but will > try to fix it next week. No worries. Sending to cq. https://codereview.chromium.org/2427633004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/payments/PaymentRequest.cpp (right): https://codereview.chromium.org/2427633004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:296: // exception. On 2016/10/18 14:13:00, Mathieu Perreault wrote: > It took me a few passes to understand this pattern. Perhaps you could expand on > the comment here or at the callsite. Also it's a bit jarring to see exceptions > being thrown and "return true;" right after, but I understand the need... Done.
The CQ bit was checked by rouslan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mathp@chromium.org, haraken@chromium.org Link to the patchset: https://codereview.chromium.org/2427633004/#ps80001 (title: "Clarify the intent of the code. Add a TODO depending on GC fix.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #2 (id:80001)
Message was sent while issue was closed.
I've now fixed the oilpan bug in https://chromiumcodereview.appspot.com/2436123002.
Message was sent while issue was closed.
Description was changed from ========== Don't overwrite PaymentDetails Overwriting shipping options in PaymentDetails causes a double-delete due to the bug in Oilpan GC http://crbug.com/601193. A workaround until that bug is fixed is to avoid overwriting shipping options in PaymentDetails. However, shipping options cannot be sent to the browser as-is. Therefore, the shipping options are overwritten in the Mojo data structure instead. BUG=600080, 601193 ========== to ========== Don't overwrite PaymentDetails Overwriting shipping options in PaymentDetails causes a double-delete due to the bug in Oilpan GC http://crbug.com/601193. A workaround until that bug is fixed is to avoid overwriting shipping options in PaymentDetails. However, shipping options cannot be sent to the browser as-is. Therefore, the shipping options are overwritten in the Mojo data structure instead. BUG=600080, 601193 Committed: https://crrev.com/4d6514453bd381827b30e0e1fcb15fb211387a51 Cr-Commit-Position: refs/heads/master@{#426229} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/4d6514453bd381827b30e0e1fcb15fb211387a51 Cr-Commit-Position: refs/heads/master@{#426229} |
