Bug 110677 - IndexedDB: Histogram all exits from IDBBackingStore::open
Summary: IndexedDB: Histogram all exits from IDBBackingStore::open
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: David Grogan
URL:
Keywords:
Depends on: 110675
Blocks: 110820
  Show dependency treegraph
 
Reported: 2013-02-22 18:23 PST by David Grogan
Modified: 2013-02-28 11:06 PST (History)
6 users (show)

See Also:


Attachments
Patch (3.17 KB, patch)
2013-02-22 18:33 PST, David Grogan
no flags Details | Formatted Diff | Diff
Patch (3.16 KB, patch)
2013-02-22 19:49 PST, David Grogan
no flags Details | Formatted Diff | Diff
Patch for landing (3.25 KB, patch)
2013-02-28 10:24 PST, David Grogan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Grogan 2013-02-22 18:23:33 PST
IndexedDB: Assert that IDBBackingStore::open's fallthrough path is not entered
Comment 1 David Grogan 2013-02-22 18:33:35 PST
Created attachment 189890 [details]
Patch
Comment 2 David Grogan 2013-02-22 18:43:09 PST
Josh or Alec, could you take a look?

IO error on schema check and failed openInMemory were the last two situations caught by the fallthrough that is now histogrammed as UnknownErr.
Comment 3 Joshua Bell 2013-02-22 19:33:39 PST
Comment on attachment 189890 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=189890&action=review

lgtm

> Source/WebCore/Modules/indexeddb/IDBBackingStore.cpp:374
> +        if (db)

Restructure this as an early exit to reduce the amount of nesting?

if (!db) {
  blah;
  return;
}
blah;

... but it's not a huge win here.
Comment 4 David Grogan 2013-02-22 19:48:52 PST
Comment on attachment 189890 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=189890&action=review

>> Source/WebCore/Modules/indexeddb/IDBBackingStore.cpp:374
>> +        if (db)
> 
> Restructure this as an early exit to reduce the amount of nesting?
> 
> if (!db) {
>   blah;
>   return;
> }
> blah;
> 
> ... but it's not a huge win here.

Done.
Comment 5 David Grogan 2013-02-22 19:49:46 PST
Created attachment 189901 [details]
Patch
Comment 6 David Grogan 2013-02-22 19:50:35 PST
Tony, could you review this?
Comment 7 Build Bot 2013-02-22 21:56:49 PST
Comment on attachment 189901 [details]
Patch

Attachment 189901 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16714498

New failing tests:
fast/css/sticky/sticky-both-sides.html
fast/css/sticky/inline-sticky.html
Comment 8 Tony Chang 2013-02-25 09:52:29 PST
Comment on attachment 189901 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=189901&action=review

> Source/WebCore/ChangeLog:8
> +        Reviewed by NOBODY (OOPS!).
> +
> +        * Modules/indexeddb/IDBBackingStore.cpp:

Nit: You should still have a line here saying why there is no test.  E.g., No tests because this only changes logging code.  In the long run, we may want to come up with a way to write a test for this in webkit_unit_tests.
Comment 9 David Grogan 2013-02-28 10:24:57 PST
Created attachment 190751 [details]
Patch for landing
Comment 10 David Grogan 2013-02-28 10:26:05 PST
(In reply to comment #8)
> (From update of attachment 189901 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=189901&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +        Reviewed by NOBODY (OOPS!).
> > +
> > +        * Modules/indexeddb/IDBBackingStore.cpp:
> 
> Nit: You should still have a line here saying why there is no test.  E.g., No tests because this only changes logging code.  In the long run, we may want to come up with a way to write a test for this in webkit_unit_tests.

Done.
Comment 11 WebKit Review Bot 2013-02-28 11:06:05 PST
Comment on attachment 190751 [details]
Patch for landing

Clearing flags on attachment: 190751

Committed r144330: <http://trac.webkit.org/changeset/144330>
Comment 12 WebKit Review Bot 2013-02-28 11:06:09 PST
All reviewed patches have been landed.  Closing bug.