Bug 110677 – IndexedDB: Histogram all exits from IDBBackingStore::open

• Component: New Bugs
• Status: RESOLVED FIXED

[David Grogan]

IndexedDB: Assert that IDBBackingStore::open's fallthrough path is not entered

[David Grogan]

Created attachment 189890 [details]
Patch

[David Grogan]

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.

[Joshua Bell]

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.

[David Grogan]

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.

[David Grogan]

Created attachment 189901 [details]
Patch

[David Grogan]

Tony, could you review this?

[Build Bot]

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

[Tony Chang]

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.

[David Grogan]

Created attachment 190751 [details]
Patch for landing

[David Grogan]

(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.

[WebKit Review Bot]

Comment on attachment 190751 [details]
Patch for landing

Clearing flags on attachment: 190751

Committed r144330: <http://trac.webkit.org/changeset/144330>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.