Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#6189 closed Bug (fixed)

Reducing the code size

Reported by: Sa'ar Zac Elias Owned by: Sa'ar Zac Elias
Priority: Normal Milestone: CKEditor 3.4.2
Component: General Version:
Keywords: Doc? Cc:

Description (last modified by Sa'ar Zac Elias)

A part of a discussion with Fred:

Clearly the code is constantly getting bigger. So how about reviewing the code and try to reduce it: use 1 and 0 for true and false when possible, remove the "defined" configuration when possible (leaving just comments about them of course) etc.

Attachments (2)

6189.patch (107.1 KB) - added by Sa'ar Zac Elias 7 years ago.
6189_2.patch (114.7 KB) - added by Sa'ar Zac Elias 7 years ago.

Download all attachments as: .zip

Change History (16)

comment:1 Changed 7 years ago by Sa'ar Zac Elias

Description: modified (diff)
Status: newassigned

comment:2 Changed 7 years ago by Alfonso Martínez de Lizarrondo

I think that all the true/false -> 1/0 conversions should be done at package time. That way the code remains as is and it will be future-proof.

comment:3 Changed 7 years ago by Frederico Caldeira Knabben

@alfonsoml, I think we're using the strict equality operator in some places, which would break things. Maybe better to have the 1/0 thing is a coding style best practice, so we have better control of it.

Changed 7 years ago by Sa'ar Zac Elias

Attachment: 6189.patch added

comment:4 Changed 7 years ago by Sa'ar Zac Elias

Status: assignedreview

These changes should become routine for the core developers as well as the contributers, so adding documentation regarding flags and initializing of config settings is recommended IMHO.

comment:5 Changed 7 years ago by Garry Yao

Isn't this suppose to be a [MicroChanges micro-change]?

comment:6 Changed 7 years ago by Sa'ar Zac Elias

That was my initial thought, but Fred asked me to open a ticket for that and put the patch on review.

comment:7 Changed 7 years ago by Garry Yao

Status: reviewreview_failed

In which sense does the lazy config option initialization style contribute to performance? Anway some of the styles configs are also used by 'pastefromword' plugin thus get broken. Besides, some of the changes in the patch are CKPackager's task instead, e.g. quotes transformation, simple block braces, check this out.

comment:8 Changed 7 years ago by Garry Yao

For me this's pretty much a packager ticket really, the 1st feature I would vote would be an invariant variable feature that caches reference like editor.lang.common.generalTab will contribute a lot to the reduction.

Changed 7 years ago by Sa'ar Zac Elias

Attachment: 6189_2.patch added

comment:9 in reply to:  7 ; Changed 7 years ago by Sa'ar Zac Elias

Status: review_failedreview

Replying to garry.yao:

Besides, some of the changes in the patch are CKPackager's task instead, e.g. quotes transformation, simple block braces, check this out.

These are just some coding style fixes I added.

the 1st feature I would vote would be an invariant variable feature that caches reference like editor.lang.common.generalTab will contribute a lot to the reduction.

I agree, that could be great, but let's do it ourselves in the meanwhile.

comment:10 in reply to:  9 Changed 7 years ago by Garry Yao

the 1st feature I would vote would be an invariant variable feature that caches reference like editor.lang.common.generalTab will contribute a lot to the reduction.

I agree, that could be great, but let's do it ourselves in the meanwhile.

Moved to #6475, so don't worry about it now.

comment:11 in reply to:  5 Changed 7 years ago by Garry Yao

Status: reviewreview_passed

1kb of footprint reduction is produced after the patch.

comment:12 Changed 7 years ago by Sa'ar Zac Elias

Keywords: Doc? added
Resolution: fixed
Status: review_passedclosed

Fixed with [5949]. Additional minor fixes should be commited as micro changes.
These small things should become routine, let's have a documentation for them.

comment:13 Changed 7 years ago by Sa'ar Zac Elias

Milestone: CKEditor 3.5CKEditor 3.4.2

comment:14 Changed 7 years ago by Frederico Caldeira Knabben

A note... it was wrong to have such big change in a micro release. That's why it has been targeted to the 3.5 earlier. More attention is required in the future.

Another note... just to remember: the "this" trick is not needed. CKPackager automatically replaces "this" if there are enough occurrences of it to compensate it. Anyway, there is no need to change what has already been changed.

Note: See TracTickets for help on using tickets.
© 2003 – 2017 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy