[webkit-reviews] review canceled: [Bug 28918] Add support for Database.readTransaction() : [Attachment 39098] patch

[bugzilla-daemon at webkit.org]

Dumitru Daniliuc <dumi at chromium.org> has canceled Dumitru Daniliuc
<dumi at chromium.org>'s request for review:
Bug 28918: Add support for Database.readTransaction()
https://bugs.webkit.org/show_bug.cgi?id=28918

Attachment 39098: patch
https://bugs.webkit.org/attachment.cgi?id=39098&action=review

------- Additional Comments from Dumitru Daniliuc <dumi at chromium.org>
Same patch, with resolved ChangeLog files.

(In reply to comment #8)
> With this change we're changing the semantics of what 'readOnly' currently
> means to the authorizer. 
> 
> The existing behavior is to support safari's private browsing mode and
> generally (but no exactly) constrains any side effects of a statement to not
be
> visible outside of the connection to the database. Accordingly, creating and
> deleting 'temp' objects only visible on that connection are allowable for
> private browsing. The new readonly transaction semantics constrain any side
> effects to not be visible outside of a transaction, which also excludes
> creating and deleting 'temp' objects. There are also differences in how VIEWS

> are treated, for private browsing purposes these can be created and deleted,
> but in readonly transaction semantics these schema changes can't be made. 
> 
> Unless we can reconcile the two meanings of 'readOnly', we may need two
> separate flags.

I talked to Brady about this. The only differences were the REINDEX and
(CREATE|DROP) [TEMP] VIEW commands. According to Brady, REINDEX was an omission
and should've been fixed, and since VIEWs result in new temp files, they have
no place in private browsing. Basically, as the flag (readOnlyMode) in the code
suggests, private browsing should really be a read-only mode. So I believe the
subsets of allowed operations in readTransaction() and private browsing are
identical.

> 91 static JSValue DatabaseTransactionHelper(... RefPtr<Database> database,
bool
> readOnly)
> 
> I think Database* would be better.

done.

> 104	  executeStatement(tx, "DROP TABLE TestTempTable;",
> "SQLITE_DROP_TEMP_TABLE");
> 105	  executeStatement(tx, "DROP TRIGGER TestTempTrigger;",
> "SQLITE_DROP_TEMP_TRIGGER");
> 106	  executeStatement(tx, "DROP VIEW TestTempView;",
> "SQLITE_DROP_TEMP_VIEW");
> 
> These lines look like they intend to operate on TEMP objects, but I don't
think
> they actually do.

They do, these entities are created using 'CREATE TEMP <something>'. The syntax
for dropping TEMP tables/triggers/views is the same as the syntax for dropping
permanent entities.