Created attachment 234127 [details] [review]
An an edit mode for google documents

I absolutely love this. I would even go as far as to use it by default for google documents as the PDF exported versions always feel like a 2nd class view. I don't consider the editing part to be the important bit, just a 'first class' rendering of the document is what I like about this.

Created attachment 234146 [details] [review]
An an edit mode for google documents

Created attachment 234149 [details] [review]
An an edit mode for google documents

Fix logic bug with fullscreen

Review of attachment 234149 [details] [review]:

Thanks Jon, this looks great! I put some comments below

::: src/edit.js
@@ +40,3 @@
+const View = imports.view;
+
+const _FULLSCREEN_TOOLBAR_TIMEOUT = 2; // seconds

This isn't needed here

@@ +57,3 @@
+
+        this._session = WebKit.get_default_session ();
+        Soup.Session.prototype.add_feature.call(this._session, new Soup.ProxyResolverDefault());

Ouch, this._session.add_feature() doesn't work?

@@ +58,3 @@
+        this._session = WebKit.get_default_session ();
+        Soup.Session.prototype.add_feature.call(this._session, new Soup.ProxyResolverDefault());
+        Soup.Session.prototype.remove_feature.call(this._session, new Soup.CookieJar());

Is this really needed? Why do we have to call remove_feature() with an object we just created? Maybe we need to call remove_feature_by_type() instead?

@@ +92,3 @@
+
+    _onProgressChanged: function() {
+        if (!this.view.uri || this.view.uri == "about:blank")

Use a define for about:blank, since it appears twice.

@@ +125,3 @@
+
+        if (uri == null)
+            uri = 'about:blank';

I'd check for if(!uri), since that also catches empty/undefined values.

@@ +183,3 @@
+    },
+
+    setModel: function(model) {

This isn't used here.

::: src/embed.js
@@ +398,3 @@
+            function() {
+                Application.modeController.setWindowMode(WindowMode.WindowMode.PREVIEW);
+                if (!doc)

I'd rather move all the logic related to these actions in edit.js directly. Note that you can connect to the load signals from there too, using Application.documentManager

@@ +488,3 @@
+            if (oldMode == WindowMode.WindowMode.EDIT) {
+                Application.documentManager.reloadActiveItem();
+            }

No braces for single-line blocks

@@ +533,3 @@
+            if (doc instanceof Documents.GoogleDocument) {
+                this._editAction.enabled = true;
+            }

No braces here

@@ +547,3 @@
     _prepareForOverview: function() {
+        if (this._preview) {
+            this._preview.controlsVisible = false;

This shouldn't be needed, preview.setModel() should already reset it to false.

@@ +585,3 @@
+    _prepareForEdit: function() {
+        if (this._preview) {
+            this._preview.controlsVisible = false;

Can't you call setModel(null) here as well?

Review of attachment 234149 [details] [review]:

Thanks Jon, this looks great! I put some comments below

::: src/edit.js
@@ +40,3 @@
+const View = imports.view;
+
+const _FULLSCREEN_TOOLBAR_TIMEOUT = 2; // seconds

This isn't needed here

@@ +57,3 @@
+
+        this._session = WebKit.get_default_session ();
+        Soup.Session.prototype.add_feature.call(this._session, new Soup.ProxyResolverDefault());

Ouch, this._session.add_feature() doesn't work?

@@ +58,3 @@
+        this._session = WebKit.get_default_session ();
+        Soup.Session.prototype.add_feature.call(this._session, new Soup.ProxyResolverDefault());
+        Soup.Session.prototype.remove_feature.call(this._session, new Soup.CookieJar());

Is this really needed? Why do we have to call remove_feature() with an object we just created? Maybe we need to call remove_feature_by_type() instead?

@@ +92,3 @@
+
+    _onProgressChanged: function() {
+        if (!this.view.uri || this.view.uri == "about:blank")

Use a define for about:blank, since it appears twice.

@@ +125,3 @@
+
+        if (uri == null)
+            uri = 'about:blank';

I'd check for if(!uri), since that also catches empty/undefined values.

@@ +183,3 @@
+    },
+
+    setModel: function(model) {

This isn't used here.

::: src/embed.js
@@ +398,3 @@
+            function() {
+                Application.modeController.setWindowMode(WindowMode.WindowMode.PREVIEW);
+                if (!doc)

I'd rather move all the logic related to these actions in edit.js directly. Note that you can connect to the load signals from there too, using Application.documentManager

@@ +488,3 @@
+            if (oldMode == WindowMode.WindowMode.EDIT) {
+                Application.documentManager.reloadActiveItem();
+            }

No braces for single-line blocks

@@ +533,3 @@
+            if (doc instanceof Documents.GoogleDocument) {
+                this._editAction.enabled = true;
+            }

No braces here

@@ +547,3 @@
     _prepareForOverview: function() {
+        if (this._preview) {
+            this._preview.controlsVisible = false;

This shouldn't be needed, preview.setModel() should already reset it to false.

@@ +585,3 @@
+    _prepareForEdit: function() {
+        if (this._preview) {
+            this._preview.controlsVisible = false;

Can't you call setModel(null) here as well?

Created attachment 234158 [details] [review]
An an edit mode for google documents

Review of attachment 234158 [details] [review]:

Looks good!

Attachment 234158 [details] pushed as b8b207a - An an edit mode for google documents