Mobwrite #198

NeilFraser · 2018-06-27T23:24:08Z

No description provided.

The open-source repo is dead, we are adopting the codebase and forking it. This is not third-party code.

It is commented out, but all editors share with the same key, ‘Code’.

Thus MobWrite can set source without indicating that the new source is saved.

- Only load MobWrite on demand. - Generate unique sharing key. - Add sharing dialog to enable/disable sharing. - Place sharing key on URL.

Previously they filled the more restrictive HTML ID limits.

cpcallen · 2018-06-28T21:29:08Z

third_party/DMP/diff_match_patch_uncompressed.js

@@ -26,7 +26,7 @@
 * Class containing the diff, match and patch methods.
 * @constructor
 */
-function diff_match_patch() {
+var diff_match_patch = function() {


If this is a class, shouldn't it be called DiffMatchPatch? (I am aware that it is not feasible to change it now…)

Yes, the name was chosen pre-Google. Stuck with it now.

cpcallen · 2018-06-28T21:40:47Z

etc/cc-dev.conf

+    client_max_body_size       10m;
+    client_body_buffer_size    128k;
+
+    proxy_connect_timeout      90;


Isn't 90s kind of long? The manual says "It should be noted that this timeout cannot usually exceed 75 seconds." The default is 60s; is there any reason to increase it? (I'd probably reduce it to 10s or something, so users get a nice nginx-generated error page instead of just waiting.

cpcallen · 2018-06-28T21:40:52Z

etc/cc-dev.conf

+
+    proxy_connect_timeout      90;
+    proxy_send_timeout         90;
+    proxy_read_timeout         90;


It looks like nginx can take a units suffix (at least for seconds). If that's correct, lets write all these timeouts as as 90s (or whatever value is chosen) for clarity.

cpcallen · 2018-06-28T21:51:18Z

etc/cc-dev.conf

@@ -26,9 +21,6 @@ server {
    proxy_pass         http://localhost:7781/login;
    proxy_redirect     off;

-    proxy_set_header   Host             $host;
-    proxy_set_header   X-Real-IP        $remote_addr;
-    proxy_set_header   X-Forwarded-For  $proxy_add_x_forwarded_for;
    proxy_max_temp_file_size 0;

    client_max_body_size       10m;


Let's not cargo-cult this stuff either.

The defaults for most of these settings seems fine, so I've deleted a bunch of them.

cpcallen

I’ve not reviewed DMP, since it’s third_party. As far as MobWrite goes:

Have you run gpylint (or at least pylint) on all the .py files?
Many python functions / methods missing docstrings. These can be omitted for ones which are "not externally visible, very short and obvious" (but note that any internal names should begin with _). Where they are included, do note:

The docstring should be descriptive ("""Fetches rows from a Bigtable.""") rather than imperative ("""Fetch rows from a Bigtable.""”).

cpcallen · 2018-06-28T21:54:28Z

mobwrite/diff_match_patch.py

@@ -0,0 +1,1842 @@
+#!/usr/bin/python2.4


File should be symlinked to existing copy.

cpcallen · 2018-06-28T21:57:20Z

etc/cc-dev.conf

-    # Things we'll probably want later:
-    # proxy_set_header   Host             $host;
-    # proxy_set_header   X-Real-IP        $remote_addr;
-    # proxy_set_header   X-Forwarded-For  $proxy_add_x_forwarded_for;


Agree with removing this for privacy reasons as discussed, but maybe replace with a comment pointing out the importance of not sending any unnecessary info to server.

cpcallen · 2018-06-28T22:06:08Z

mobwrite/mobwrite_server.py

@@ -0,0 +1,471 @@
+#!/usr/bin/python2.4


cpcallen · 2018-06-29T18:50:58Z

mobwrite/mobwrite_core_test.py

@@ -0,0 +1,83 @@
+#!/usr/bin/python2.4


Styleguide:

Start the main file of a non-google3 non-prod program with #!/usr/bin/python with an optional single digit 2 or 3 suffix per PEP-394.

cpcallen · 2018-06-29T18:55:12Z

mobwrite/mobwrite_config.txt

@@ -0,0 +1,33 @@
+; ---------------------
+; Settings for MobWrite
+; ---------------------


This file ought to be named mobwrite.cfg or the like, rather than being a .txt (since it is not merely arbitrary human-readable text).

cpcallen · 2018-07-03T01:15:22Z

static/code/editor.js

+  if (Code.Editor.currentEditor) {
+    value = Code.Editor.currentEditor.getSource() || '';
+  }
+  // Numeric data should use overwrite mode.


https://code.google.com/archive/p/google-mobwrite/wikis/Numeric.wiki

cpcallen · 2018-07-03T01:17:42Z

static/code/mobwrite/mobwrite_core.js

-    if (mobwrite.debug) {
-      window.console.info('Creating shareObj: "' + id + '"');
-    }
+  if (!id) return;  // Creating prototype object.


"Creating subclass prototype object"?

cpcallen · 2018-07-03T01:25:10Z

static/code/editor.js

   */
-  this.lastSavedSource_ = null;
+  this.lastSavedSource = null;


Like most of the properties you create on Code.Editor.protoype, this should be created instead in the Code.Editor constructor.

Huh? It is.

cpcallen · 2018-07-03T01:34:04Z

static/code/editor.js

+    for (var i = 0; i < files.length; i++) {
+      var script = document.createElement('script');
+      script.src = 'mobwrite/' + files[i];
+      document.head.appendChild(script);


Doesn't this actually load the script synchronously, in contradiction to the JSDoc above?

Injecting a script is synchronous if it is placed ahead of the loading wave-front. In this case the document is fully-loaded (which is the point of lazy-loading), so everything is async.

cpcallen · 2018-07-03T01:37:00Z

static/code/editor.js

+      typeof mobwrite === 'undefined' ||
+      typeof Code.mobwriteShare === 'undefined') {
+    // Not loaded, try again later.
+    setTimeout(Code.Editor.waitMobWrite_, 50, callback);


Is polling really the only way to do this? Surely these scripts could check the value of a well-known global variable and, if it is a function, call it? (That would entail the ugliness of a global variable, yes, but better than polling.)

The scripts could be loaded in any order. It would be inappropriate for mobwrite_core.js or dmp.js to have that sort of hook.

cpcallen · 2018-07-04T18:37:40Z

I am approving this, but not your python style. Filed #201 to address this at an appropriate future date.

# Conflicts: # .gitignore

NeilFraser added 7 commits June 19, 2018 13:25

Update DMP, add Python.

09e56ec

Add MobWrite

0d4a188

The open-source repo is dead, we are adopting the codebase and forking it. This is not third-party code.

Fix async editor loads with empty string.

7e456d8

Share editor contents with MobWrite.

c950637

It is commented out, but all editors share with the same key, ‘Code’.

Add isSaved param to setSourceToAllEditors

a9c8074

Thus MobWrite can set source without indicating that the new source is saved.

Make MobWrite usable

7ec1bb7

- Only load MobWrite on demand. - Generate unique sharing key. - Add sharing dialog to enable/disable sharing. - Place sharing key on URL.

Expand random IDs to fill URL hash limits.

69b2f6b

Previously they filled the more restrictive HTML ID limits.

NeilFraser requested a review from cpcallen June 27, 2018 23:24

cpcallen reviewed Jun 28, 2018

View reviewed changes

NeilFraser added 2 commits June 28, 2018 14:55

Properly simlink DMP.py

61e1c4e

Delete unneeded cargoculted nginx config

68df13c

cpcallen requested changes Jul 3, 2018

View reviewed changes

Updates from code review.

507b1f4

cpcallen mentioned this pull request Jul 4, 2018

Address python style issues in MobWrite server #201

Open

cpcallen approved these changes Jul 4, 2018

View reviewed changes

cpcallen and others added 2 commits July 6, 2018 15:17

Ignore .city.partial files

9ef3674

# Conflicts: # .gitignore

Merge branch 'master' into mobwrite

283ef20

NeilFraser merged commit 24dc821 into master Jul 6, 2018

NeilFraser deleted the mobwrite branch July 6, 2018 22:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mobwrite #198

Mobwrite #198

NeilFraser commented Jun 27, 2018

cpcallen Jun 28, 2018

NeilFraser Jun 28, 2018

cpcallen Jun 28, 2018 •

edited

NeilFraser Jun 30, 2018

cpcallen Jun 28, 2018

NeilFraser Jun 30, 2018

cpcallen Jun 28, 2018

NeilFraser Jun 30, 2018

cpcallen left a comment

cpcallen Jun 28, 2018

cpcallen Jun 28, 2018

cpcallen Jun 28, 2018

NeilFraser Jul 4, 2018

cpcallen Jun 29, 2018

NeilFraser Jul 4, 2018

cpcallen Jun 29, 2018

NeilFraser Jul 4, 2018

cpcallen Jul 3, 2018

NeilFraser Jul 4, 2018

cpcallen Jul 3, 2018

NeilFraser Jul 4, 2018

cpcallen Jul 3, 2018

NeilFraser Jul 4, 2018

cpcallen Jul 3, 2018

NeilFraser Jul 4, 2018

cpcallen Jul 3, 2018

NeilFraser Jul 4, 2018

cpcallen commented Jul 4, 2018

Mobwrite #198

Mobwrite #198

Conversation

NeilFraser commented Jun 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpcallen Jun 28, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpcallen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpcallen commented Jul 4, 2018

cpcallen Jun 28, 2018 •

edited