Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(123)

Issue 2751953002: sendBeacon(): once transmission allowance has been reached, always fail. (Closed)

Created:
3 years, 9 months ago by sof
Modified:
3 years, 9 months ago
CC:
blink-reviews, chromium-reviews
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

sendBeacon(): once transmission allowance has been reached, always fail. Fix allowance checking logic for Beacon transmissions upon reaching the limit. If the allowance limit was 'perfectly' exhausted after N Beacon requests, subsequent Beacon requests would go ahead without the (now zero) allowance limit imposed. R=mkwst BUG=701678 Review-Url: https://codereview.chromium.org/2751953002 Cr-Commit-Position: refs/heads/master@{#457088} Committed: https://chromium.googlesource.com/chromium/src/+/c1a211aa583ade023aca652e9493c01603623aa3

Patch Set 1 #

Patch Set 2 : fix test description imprecision #

Unified diffs Side-by-side diffs Delta from patch set Stats (+18 lines, -1 line) Patch
A third_party/WebKit/LayoutTests/http/tests/sendbeacon/beacon-allowance-limit.html View 1 1 chunk +17 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/loader/PingLoader.cpp View 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 27 (13 generated)
sof
please take a look.
3 years, 9 months ago (2017-03-15 10:39:32 UTC) #4
Mike West
Ha! Nice find. LGTM!
3 years, 9 months ago (2017-03-15 14:56:36 UTC) #11
Mike West
(I'm assuming this is a Chrome thing and not a specced limit? If I'm wrong, ...
3 years, 9 months ago (2017-03-15 14:57:30 UTC) #12
sof
On 2017/03/15 14:57:30, Mike West (Slow.) wrote: > (I'm assuming this is a Chrome thing ...
3 years, 9 months ago (2017-03-15 15:25:12 UTC) #13
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2751953002/20001
3 years, 9 months ago (2017-03-15 15:26:02 UTC) #16
commit-bot: I haz the power
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/c1a211aa583ade023aca652e9493c01603623aa3
3 years, 9 months ago (2017-03-15 15:31:27 UTC) #19
tyoshino (SeeGerritForStatus)
lgtm
3 years, 9 months ago (2017-03-15 15:35:47 UTC) #20
tyoshino (SeeGerritForStatus)
On 2017/03/15 15:35:47, tyoshino wrote: > lgtm Seems we should fix maxBeaconTransmission to be unsigned ...
3 years, 9 months ago (2017-03-15 15:38:16 UTC) #21
sof
On 2017/03/15 15:38:16, tyoshino wrote: > On 2017/03/15 15:35:47, tyoshino wrote: > > lgtm > ...
3 years, 9 months ago (2017-03-15 15:45:05 UTC) #22
tyoshino (SeeGerritForStatus)
On 2017/03/15 15:45:05, sof wrote: > On 2017/03/15 15:38:16, tyoshino wrote: > > On 2017/03/15 ...
3 years, 9 months ago (2017-03-15 15:48:53 UTC) #23
tyoshino (SeeGerritForStatus)
On 2017/03/15 15:48:53, tyoshino wrote: > On 2017/03/15 15:45:05, sof wrote: > > On 2017/03/15 ...
3 years, 9 months ago (2017-03-15 15:49:45 UTC) #24
sof
On 2017/03/15 15:49:45, tyoshino wrote: > On 2017/03/15 15:48:53, tyoshino wrote: > > On 2017/03/15 ...
3 years, 9 months ago (2017-03-15 16:07:23 UTC) #25
tyoshino (SeeGerritForStatus)
On 2017/03/15 16:07:23, sof wrote: > I think that was the intent at some point, ...
3 years, 9 months ago (2017-03-15 16:08:25 UTC) #26
sof
3 years, 9 months ago (2017-03-16 10:57:11 UTC) #27
Message was sent while issue was closed.
On 2017/03/15 16:08:25, tyoshino wrote:
> On 2017/03/15 16:07:23, sof wrote:
> > I think that was the intent at some point, to have an embedder be able to
set
> a
> > negative max to opt out of any limit. But the implementation doesn't support
> > that, so unless we feel that's valuable, switching to 'unsigned' makes good
> > sense.
> 
> OK. Anyway, let's decide it at some point and document in Settings.json5 :)

https://codereview.chromium.org/2753863003/ is my proposal

Powered by Google App Engine
This is Rietveld 408576698