Issue 2349413002: Minor IndexedDB refactorings.

pwnall

The CQ bit was checked by pwnall@chromium.org to run a CQ dry run

4 years, 3 months ago (2016-09-19 20:46:41 UTC) #1

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2349413002/1

4 years, 3 months ago (2016-09-19 20:47:30 UTC) #2

pwnall

pwnall@chromium.org changed reviewers: + cmumford@chromium.org

4 years, 3 months ago (2016-09-19 20:49:59 UTC) #3

pwnall

This has some of the code changes in http://crrev.com/2314933005 but does not introduce (significant) behavior ...

4 years, 3 months ago (2016-09-19 20:50:00 UTC) #4

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 3 months ago (2016-09-19 22:50:14 UTC) #5

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

4 years, 3 months ago (2016-09-19 22:50:14 UTC) #6

cmumford

Lots of little nit-pickey comments. Still looking at some files... https://codereview.chromium.org/2349413002/diff/1/third_party/WebKit/Source/modules/indexeddb/IDBIndex.cpp File third_party/WebKit/Source/modules/indexeddb/IDBIndex.cpp (right): https://codereview.chromium.org/2349413002/diff/1/third_party/WebKit/Source/modules/indexeddb/IDBIndex.cpp#newcode55 ...

4 years, 3 months ago (2016-09-19 23:26:48 UTC) #7

pwnall

The CQ bit was checked by pwnall@chromium.org to run a CQ dry run

4 years, 3 months ago (2016-09-20 09:09:45 UTC) #8

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2349413002/20001

4 years, 3 months ago (2016-09-20 09:10:02 UTC) #9

pwnall

cmumford: Thank you for the quick feedback. I have addressed some of it, and asked ...

4 years, 3 months ago (2016-09-20 09:11:15 UTC) #10

cmumford: Thank you for the quick feedback. I have addressed some of it, and
asked questions where I got confused. PTAL?

https://codereview.chromium.org/2349413002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/indexeddb/IDBIndex.cpp (right):

https://codereview.chromium.org/2349413002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/indexeddb/IDBIndex.cpp:55: DCHECK(id() !=
IDBIndexMetadata::InvalidId);
On 2016/09/19 23:26:48, cmumford wrote:
> DCHECK_NE

Done.

TBH, I was trying to be smart and thinking that the message wouldn't be so
useful. But it's much easier (as a rule) to do DCHECK_NE whenever possible, so I
did that in all the files I changed.

https://codereview.chromium.org/2349413002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/indexeddb/IDBIndex.cpp:68: void
IDBIndex::setName(const String& newName, ExceptionState& exceptionState)
On 2016/09/19 23:26:48, cmumford wrote:
> Nit: Several other Blink "setName" methods use "name" for this parameter. The
> only other place in Blink where "newName" is used is when there is also an
> "oldName" variable. Was this just done for clarity?
> 
> Note: Just a minor nit, I don't feel strongly about this, but the prior
> implementation does seem to conform better with other Blink code.

Done.

I was mostly trying to avoid if (this->name() == name)

https://codereview.chromium.org/2349413002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/indexeddb/IDBIndex.h (right):

https://codereview.chromium.org/2349413002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/indexeddb/IDBIndex.h:56: const
IDBIndexMetadata& metadata() const { return m_metadata; }
On 2016/09/19 23:26:48, cmumford wrote:
> I also don't see the need for this method. Seems to just add another layer of
> indirection and is only used by IDBIndex - so could be private.

The main benefit here is also being able to switch the metadata type without as
much code churn.

I made the method private. Is this reasonable?

https://codereview.chromium.org/2349413002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/indexeddb/IDBObjectStore.cpp (right):

https://codereview.chromium.org/2349413002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/indexeddb/IDBObjectStore.cpp:62: using
IndexKeys = HeapVector<Member<IDBKey>>;
On 2016/09/19 23:26:48, cmumford wrote:
> I'm OK with defining |IndexKeys|, and would also be OK with avoiding the type
> and just using HeapVector<Member<IDBKey>> as it isn't much longer.

If you're OK with defining it, I would rather go that route. My main concern is
"HeapVector<IndexKeys> indexKeys;", which would be a bit more difficult to read
if IndexKeys were to be inlined.

FWIW, I only moved this definition away from the header because no other file
was using it.

https://codereview.chromium.org/2349413002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/indexeddb/IDBObjectStore.cpp:530: const
IDBIndexMetadata& indexMetadata() { return m_indexMetadata; }
On 2016/09/19 23:26:48, cmumford wrote:
> const.

Done.

Thank you! There should be a warning for that :D

https://codereview.chromium.org/2349413002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/indexeddb/IDBObjectStore.cpp:844: IDBIndex*
index = m_indexMap.take(oldName);
On 2016/09/19 23:26:48, cmumford wrote:
> Nit: How about:
> 
>   m_indexMap.set(newName, m_indexMap.take(oldName));

Done.
I made this change in IDBTransaction::objectStoreRenamed as well.

https://codereview.chromium.org/2349413002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/indexeddb/IDBObjectStore.h (right):

https://codereview.chromium.org/2349413002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/indexeddb/IDBObjectStore.h:63: const
IDBKeyPath& idbKeyPath() const { return metadata().keyPath; }
On 2016/09/19 23:26:48, cmumford wrote:
> IDBObjectStore *is* an "idb" thing, so idbKeyPath is slightly redundant. How
> about just "keyPath"?

There's a keyPath() accessor in the WebIDL interface, which does a bit more (on
line 69). I'm slightly worried about having a method with one overload
implementing a method in the spec and the other overload used internally. If you
think there wouldn't be much confusion, I can make the switch.

https://codereview.chromium.org/2349413002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/indexeddb/IDBObjectStore.h:72: bool
autoIncrement() const { return metadata().autoIncrement; }
On 2016/09/19 23:26:48, cmumford wrote:
> What is the rationale for switching m_metadata to metadata()?

The idea is that we can switch the metadata representation (e.g., to a
RefPtr<IDBObjectStoreMetadata>, which would share the underlying structure with
the database metadata) without causing as much code churn.

https://codereview.chromium.org/2349413002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/indexeddb/IDBRequest.cpp (right):

https://codereview.chromium.org/2349413002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/indexeddb/IDBRequest.cpp:326: #if
ENABLE(ASSERT)
On 2016/09/19 23:26:48, cmumford wrote:
> If switching to DCHECK should you also switch to:
> 
>   #if DCHECK_IS_ON()

Done.

I also did a bunch more ASSERT->DCHECK changes. It seemed fitting for a
refactoring CL.

https://codereview.chromium.org/2349413002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/indexeddb/IDBTransaction.cpp (right):

https://codereview.chromium.org/2349413002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/indexeddb/IDBTransaction.cpp:52:
DCHECK(!objectStoreNames.isEmpty()) << "Non-version transactions should operate
on a well-defined set of stores";
On 2016/09/19 23:26:48, cmumford wrote:
> Add a space before 'Non'

I apologize in advance for being clueless, but can you please explain why I need
a space here?

Based on sampling, the codebase doesn't seem to use space.
https://cs.chromium.org/search/?q=DCHECK.*%3C%3C&sq=package:chromium&type=cs

If I understand the code correctly, my message will be added after a ". "
(period, space), so adding a space seems redundant.
https://cs.chromium.org/chromium/src/base/logging.h?q=DCHECK&sq=package:chrom...

Thank you in advance!

https://codereview.chromium.org/2349413002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/indexeddb/IDBTransaction.cpp:95: ,
m_scope(objectStoreNames)
On 2016/09/19 23:26:48, cmumford wrote:
> Nit: It does seem a bit odd to have a public name "objectStoreNames", but a
> different internal name: "m_scope".

I agree that it does seem weird. Here is my rationale:

(1) the spec states that a transaction has a scope:
https://w3c.github.io/IndexedDB/#transaction-scope
(2) the WebIDL for creating transactions uses the storeNames parameter name:
https://w3c.github.io/IndexedDB/#dom-idbdatabase-transaction
(3) We have an objectStoreNames method (WebIDL accessor) that sometimes returns
m_scope, and sometimes returns something else. (for versionchange)

I flipped "objectStoreNames" to "scope", and kept the "storeNames" parameter in
IDBDatabase::transaction(). I think this name is a bit better, because it maps
directly to a spec concept. Granted, it'd be pretty easy to guess what a list of
objectStoreNames is for, so the difference doesn't seem big. I'll be happy to
switch everything back to objectStoreNames if it makes more sense to you!

https://codereview.chromium.org/2349413002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/indexeddb/IDBTransaction.cpp:100: if
(isVersionChange()) {
On 2016/09/19 23:26:48, cmumford wrote:
> Is there a need to DCHECK mode?

We got by without it, but it seems like a good idea. I added a couple of DCHECKs
in this constructor, because I think it's the best place to check rep
invariants. Thank you for pointing this out!

https://codereview.chromium.org/2349413002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/indexeddb/IDBTransaction.cpp:106:
DCHECK(!objectStoreNames.isEmpty()) << "Non-versionchange transactions must
operate on a well-defined set of stores";
On 2016/09/19 23:26:48, cmumford wrote:
> Space before 'Non'

The same question as above applies. After I understand the reasoning, I'll make
a pass through all the DCHECKs I've added to IndexedDB.

https://codereview.chromium.org/2349413002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/indexeddb/IDBTransaction.cpp:181:
DCHECK_NE(m_state, Finished) << "A finished transaction created an object
store";
On 2016/09/19 23:26:48, cmumford wrote:
> spaces before these (and all) strings, so:
> 
>   DCHECK(...) << "Foo";
> 
> Should be:
> 
>   DCHECK(...) << " Foo";

The same question as above applies. After I understand the reasoning, I'll make
a pass through all the DCHECKs I've added to IndexedDB.

https://codereview.chromium.org/2349413002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/indexeddb/IDBTransaction.h (right):

https://codereview.chromium.org/2349413002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/indexeddb/IDBTransaction.h:123: void finish();
On 2016/09/19 23:26:48, cmumford wrote:
> If the transaction has already finished then this function should be named
> "finished" to avoid confusion.

Done.

https://codereview.chromium.org/2349413002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/indexeddb/IDBTransaction.h:134: const
HashSet<String> m_scope;
On 2016/09/19 23:26:48, cmumford wrote:
> You may want to add a comment above m_scope.

Done.
Thank you! That's a very good point!

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 3 months ago (2016-09-20 10:26:01 UTC) #11

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

4 years, 3 months ago (2016-09-20 10:26:01 UTC) #12

pwnall

> On 2016/09/19 23:26:48, cmumford wrote: > > spaces before these (and all) strings, so: ...

4 years, 3 months ago (2016-09-21 23:49:37 UTC) #13

cmumford

lgtm https://codereview.chromium.org/2349413002/diff/1/third_party/WebKit/Source/modules/indexeddb/IDBObjectStore.h File third_party/WebKit/Source/modules/indexeddb/IDBObjectStore.h (right): https://codereview.chromium.org/2349413002/diff/1/third_party/WebKit/Source/modules/indexeddb/IDBObjectStore.h#newcode63 third_party/WebKit/Source/modules/indexeddb/IDBObjectStore.h:63: const IDBKeyPath& idbKeyPath() const { return metadata().keyPath; } ...

4 years, 3 months ago (2016-09-23 22:21:11 UTC) #14

pwnall

https://codereview.chromium.org/2349413002/diff/1/third_party/WebKit/Source/modules/indexeddb/IDBObjectStore.h File third_party/WebKit/Source/modules/indexeddb/IDBObjectStore.h (right): https://codereview.chromium.org/2349413002/diff/1/third_party/WebKit/Source/modules/indexeddb/IDBObjectStore.h#newcode72 third_party/WebKit/Source/modules/indexeddb/IDBObjectStore.h:72: bool autoIncrement() const { return metadata().autoIncrement; } On 2016/09/23 ...

4 years, 3 months ago (2016-09-23 22:33:05 UTC) #15

pwnall

The CQ bit was checked by pwnall@chromium.org to run a CQ dry run

4 years, 3 months ago (2016-09-23 23:22:25 UTC) #16

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2349413002/60001

4 years, 3 months ago (2016-09-23 23:23:04 UTC) #17

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 3 months ago (2016-09-24 00:57:11 UTC) #18

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

4 years, 3 months ago (2016-09-24 00:57:12 UTC) #19

pwnall

The patchset sent to the CQ was uploaded after l-g-t-m from cmumford@chromium.org Link to the ...

4 years, 3 months ago (2016-09-24 01:15:15 UTC) #21

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/2349413002/60001

4 years, 3 months ago (2016-09-24 01:15:46 UTC) #22

commit-bot: I haz the power

Description was changed from ========== Minor IndexedDB refactorings. This is an extraction of all the ...

4 years, 3 months ago (2016-09-24 01:28:17 UTC) #24

commit-bot: I haz the power

4 years, 3 months ago (2016-09-24 01:28:18 UTC) #25

Message was sent while issue was closed.

Patchset 4 (id:??) landed as
https://crrev.com/b00bdc1d42f5ec166b3ab1643e6993a30e593a8c
Cr-Commit-Position: refs/heads/master@{#420806}

Issue 2349413002: Minor IndexedDB refactorings. (Closed)

Description

Patch Set 1 #

Patch Set 2 : Addressed feedback. #

Patch Set 3 : Addressed feedback. #

Patch Set 4 : Rebased #

Messages